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

feat(install): very strict global npm engines #3731

Merged
merged 1 commit into from
Sep 9, 2021

Conversation

wraithgar
Copy link
Member

This will do an engines check when installing npm globally and fail if
the new npm is known not to work in the current node version.

It will not work for older npm versions because they don't have an
engines field (it wasn't added till npm@6.14.0). It will at least
prevent npm@7 from being installed in node@8.

References

Closes #2612

@wraithgar wraithgar requested a review from a team as a code owner September 9, 2021 15:34
@ljharb
Copy link
Contributor

ljharb commented Sep 9, 2021

Can it assume that if npm lacks an engines field, it’s not compatible? That would only prevent installing npm < 6, and non-latest npm 6, right?

@wraithgar
Copy link
Member Author

npm-install-check fails open if there is no engines field. Versions of npm without an engines entry will not fail.

https://github.com/npm/npm-install-checks/blob/master/index.js#L8-L12

@ljharb
Copy link
Contributor

ljharb commented Sep 9, 2021

In general that’s obviously the right semantic - but this is a special case for npm itself. Why would we want to allow older npms to be installed by npm 7.next+?

@wraithgar
Copy link
Member Author

In general that’s obviously the right semantic - but this is a special case for npm itself. Why would we want to allow older npms to be installed by npm 7.next+?

I'm erring on the side of only taking action if we have the info to take that action.

@ljharb
Copy link
Contributor

ljharb commented Sep 9, 2021

Right, but this only applies to npm - and you have the info that any npm without an engines field is obsolete. Why allow it to be installed?

@wraithgar
Copy link
Member Author

It's more effort than it's worth. Right now we are letting the EBADENGINE message bubble up, with the intent that we let the error-messages file potentially make that error prettier in the future. If we also add this new condition we have to invent a new kind of error to throw. This is just a general failsafe before we drop the next npm version and move off of node10. This isn't intended to be comprehensive, just something that helps us going forward.

@darcyclarke darcyclarke added Release 7.x work is associated with a specific npm 7 release semver:minor new backwards-compatible feature labels Sep 9, 2021
Copy link
Contributor

@nlf nlf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good!

This will do an engines check when installing npm globally and fail if
the new npm is known not to work in the current node version.

It will not work for older npm versions because they don't have an
engines field (it wasn't added till npm@6.14.0). It will at least
prevent npm@7 from being installed in node@8.

PR-URL: #3731
Credit: @wraithgar
Close: #3731
Reviewed-by: @nlf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release 7.x work is associated with a specific npm 7 release semver:minor new backwards-compatible feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants