Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI: Change npm install to npm ci #294

Closed
wants to merge 1 commit into from
Closed

Conversation

JakobMiksch
Copy link
Collaborator

Change command to install dependencies.

@fschmenger
Copy link
Collaborator

fschmenger commented May 25, 2022

Hi @JakobMiksch,
it's generally a good idea to run the CI with a clean install, so very much appreciated.

As a consequence, we have to take care about updating the package-lock.json on a regular basis.
From the CI log I can see currently:

npm WARN old lockfile 
npm WARN old lockfile The package-lock.json file was created with an old version of npm,
npm WARN old lockfile so supplemental metadata must be fetched from the registry.
npm WARN old lockfile 
npm WARN old lockfile This is a one-time fix-up, please be patient...

Do we have any explicit ruling, which NPM version should be used at minimum?
The current package.json states

"engines": {
        "node": ">= 4.0.0",
        "npm": ">= 3.0.0"
      }

but NPM 3 is really old, so we should probably go with a newer one.

Maybe a good time now to consider this, check-in an update package-lock.json and attach it to this PR.
What do you guys think?

@JakobMiksch
Copy link
Collaborator Author

JakobMiksch commented May 25, 2022

Updating the npm/node sound reasonable to me 👍
Here an overview of Node releases https://nodejs.org/en/about/releases/
So what about requiring at least Node 14?

Also +1 for adding an updated package-lock.json

@fschmenger
Copy link
Collaborator

Sorry for being so brief in my comment. The main considerations are:

  • Is there any ruling on the minimum NPM / Node.js version so far? So if yes, we probably cannot require it before the next Wegue major release. Otherwise a newer NPM is definitely an option.
  • I'm not sure if a Node.js upgrade is needed, as the NPM and Node.js versions are not directly connected (newer NPM does not require a newer Node.js, but I'm not an expert here)
  • You could look into the CI configuration and see which NPM version is running there. I noticed that it has been complaining about an NPM version mismatch previously, so it is not this PR to blame. So if we aren't allowed to check in a package-lock.json based on a newer NPM version, then we should probably downgrade NPM in the CI.

Here's some background on the topic: https://stackoverflow.com/questions/68260784/npm-warn-old-lockfile-the-package-lock-json-file-was-created-with-an-old-version

@fschmenger
Copy link
Collaborator

Just FYI: I recently tried to update the package-lock.json. This currently breaks the production build: Under the hood OL upgrades its ol-mapbox-style dependency from 6.1.4 to 6.9.0 which doesn`t work with the current setup.

I think we should proceed with working on the integration of #297 and get back to this afterwards.

@chrismayer
Copy link
Collaborator

Closing in favor of #344

@chrismayer chrismayer closed this Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants