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

Enforce matching engines on npm install #481

Merged
merged 6 commits into from Dec 30, 2019

Conversation

sholladay
Copy link
Contributor

Fixes #480

This PR enables the engine-strict configuration for npm install when np reinstalls the dependencies. This will raise an error if the project or its dependencies have an engines.node in package.json that are incompatible with the current environment.

A question: should this be applied to the npm ci case as well? I ran npm ci --engine-strict in a package whose engines.node was incompatible with the current Node version and it did not complain, whereas npm install --engine-strict did complain in the same situation. However, I did not test what happens when dependencies have mismatched engines. I also did not test using the npm config environment variable, only the command line flag. Perhaps an implementation quirk leads to npm ci loading configuration differently.

@sindresorhus
Copy link
Owner

should this be applied to the npm ci case as well?

Yes. While there's no difference now, the problem might be fixed at some point, so it doesn't hurt to include it.

Perhaps an implementation quirk leads to npm ci loading configuration differently.

I wouldn't be surprised if this was the case. npm has a lot of weird quirks.

readme.md Outdated Show resolved Hide resolved
source/index.js Outdated Show resolved Hide resolved
sholladay and others added 2 commits December 30, 2019 17:07
Co-Authored-By: Itai Steinherz <itaisteinherz@gmail.com>
It's currently a noop for `npm ci`, presumably because of an implementation quirk. But it's what we want to do, so we'll need to look into getting npm fixed.
@sholladay
Copy link
Contributor Author

All review feedback has been addressed. :)

Copy link
Collaborator

@itaisteinherz itaisteinherz left a comment

Choose a reason for hiding this comment

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

LGTM

@itaisteinherz itaisteinherz merged commit b14dbc9 into sindresorhus:master Dec 30, 2019
@itaisteinherz
Copy link
Collaborator

Thanks @sholladay! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure dependencies are compatible with the supported Node.js version
3 participants