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

Update package-lock to v2 #29097

Closed
wants to merge 1 commit into from
Closed

Conversation

noahtallen
Copy link
Member

@noahtallen noahtallen commented Feb 18, 2021

I was trying to install a dependency the other day. I was using npm@7.5.4, and npm install was generating wild changes to the lockfile. (See the modifications to the lock-file in this PR, which are massive.) It looks like npm is updating the file format (https://docs.npmjs.com/cli/v7/configuring-npm/package-lock-json#lockfileversion), and npm 7 switches it automatically.

Gutenberg has a precommit check which requires us to use the latest npm version. However, it seems like the project is assuming npm 6 everywhere else. For example, CI runs with npm 6, and node LTS installs npm 6 by default. This isn't compatible with our precommit check, which requires package-lock changes to be committed with npm v7. I think this is creating some issues in the static CI checks.

I wanted to start a conversation about this: do we commit to npm 7 now, and update the package lock, or should we change our local tooling to require npm 6 and not npm 7?

@noahtallen noahtallen self-assigned this Feb 18, 2021
@noahtallen noahtallen added the [Type] Build Tooling Issues or PRs related to build tooling label Feb 18, 2021
@gziolo gziolo requested a review from fluiddot February 18, 2021 03:56
@jsnajdr
Copy link
Member

jsnajdr commented Feb 18, 2021

The current check-latest-npm script sends an online request to the NPM registry to figure out the latest version of the npm package and complains if you don't have it. I think that's unfortunate. This kind of check should provide a stable environment and controlled upgrades of tools. Now we have the opposite: when NPM is upgraded upstream, your local environment can be reported as obsolete although it was OK few minutes ago.

We already have a .npmrc file and a engine.npm field in package.json that says "npm": ">=6.9.0". So why don't we check against this field? There's already a wp-scripts check-engines script that does exactly that.

@youknowriad
Copy link
Contributor

youknowriad commented Feb 18, 2021

We already have a .npmrc file and a engine.npm field in package.json that says "npm": ">=6.9.0". So why don't we check against this field? There's already a wp-scripts check-engines script that does exactly that.

Because the guideline that was discussed a long time ago was not to rely on a specific version but to use the latest npm version always. That said, that guideline doesn't make sense to me though :P, it's better if the project defines what npm version it supports explicitly.

@jsnajdr
Copy link
Member

jsnajdr commented Feb 18, 2021

the guideline that was discussed a long time ago was not to rely on a specific version but to use the latest npm version always.

That was fine while the latest versions were patch releases in the 6.x.x line. Now it means a spontaneous and uncontrolled upgrade to v7 🙂

@fluiddot
Copy link
Contributor

👋 I also created some time ago a similar to PR for updating the lock file format version but the CI checks are also failing.

Looks like until NPM releases a new version with a fix we shouldn't use NPM 7, as a workaround I created a PR in case we want to limit the use of NPM 7 in the meantime.

@ockham
Copy link
Contributor

ockham commented Feb 19, 2021

Quoting #28834 (comment)

👋 Looks like the npm/cli bug related to the dependencies install error is already solved and aims to be released on NPM 7.5.4 🤞 .

I'll try rebasing this PR to restart tests.

@ockham
Copy link
Contributor

ockham commented Feb 19, 2021

Seems like we agree overall that we shouldn't depend on "latest available npm" @noahtallen @jsnajdr @youknowriad, for aforementioned reasons. Let's use either the engine.npm field from package.json, or .npmrc instead, as Jarda suggested.

@noahtallen
Copy link
Member Author

noahtallen commented Feb 19, 2021

Sounds good. I guess we just need to make sure that npm --version is the latest version in the range specified in this build script 🤔

@ockham
Copy link
Contributor

ockham commented Feb 22, 2021

Sounds good. I guess we just need to make sure that npm --version is the latest version in the range specified in this build script

@kevin940726 filed a PR to that effect: #29204 👏

@noahtallen
Copy link
Member Author

I'll close this out then :)

@noahtallen noahtallen closed this Feb 22, 2021
@youknowriad youknowriad deleted the update/package-lock-to-v2 branch February 23, 2021 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants