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

RFC for check engines requirements on every install #195

Closed
wants to merge 1 commit into from

Conversation

markablov
Copy link

Now if there is shrinkwrap file (or package-lock) NPM does not perform check if package dependencies are satisfied by provided environment (e.g. node/npm versions).

@ljharb
Copy link
Contributor

ljharb commented Aug 8, 2020

You can enable this with --engines-strict, but this should not be on by default. "engines" is purely advisory and should never fail an install.

@markablov
Copy link
Author

@ljharb yes, of course it's enabled and even if it's enabled, install does not fail if it's performed by using package-lock.json, even if wrong version of node.js is using.

@ljharb
Copy link
Contributor

ljharb commented Aug 8, 2020

Ah, then that sounds more like a bug then a new feature that requires an rfc :-) when that config flag is set, i would expect the install to check engines with or without a lockfile.

@markablov
Copy link
Author

@ljharb true, i also expected that. But because of several approaches to solve the "bug", i prefer to listen opinions of mainteners of NPM ;)

@bnb
Copy link

bnb commented Aug 19, 2020

but this should not be on by default

@ljharb I disagree with this FWIW. If I'm running Node.js 4 and my dependencies have a minimum requirement of Node.js 24, it's pretty easy to assume they're going to be using features that are not supported in my current runtime. I want to know that my app probably won't work by default rather than continuing to write code oblivious to that.

@ljharb
Copy link
Contributor

ljharb commented Aug 19, 2020

@bnb and yet, that's not actually the case in practice. Quite often, packages work fine on engines that are outside the explicitly declared range, and "engines" more serves to declare what they care to support rather than where it works or not.

@ruyadorno ruyadorno added the Agenda will be discussed at the Open RFC call label Aug 26, 2020
@isaacs
Copy link
Contributor

isaacs commented Aug 26, 2020

We check engines when reifying, and use it as a heuristic when selecting versions during the ideal tree building phase.

This should emit a warning-level log message if the engines don't match. However, I don't think it's a good idea to fail the install entirely (unless --engines-strict is enabled), or install something other than what the lockfile specifies.

If you aren't seeing those warnings emitted when installing with npm v7, then definitely post a bug about that at https://github.com/npm/cli/issues

Ref:

@isaacs
Copy link
Contributor

isaacs commented Aug 26, 2020

The suggestion that we re-check packages that aren't being installed is an interesting feature request, but unfortunately would add a significant amount of extra work in the install process.

Eg, you install foo@1.0.0 which has { "engines": {"node": ">=14"}} in the package.json. Then, later, without changing the lockfile or node_modules folder, you change your node install to v12, and run npm install again. In that case, it won't warn about the engines mismatch, or fail with --engines-strict, because it won't evaluate that part of the dep tree.

@markablov
Copy link
Author

@isaacs

a significant amount of extra work in the install process.

What about adding property to the root of lockfile? like dependencyEngines or requiredEngines, because it's not just copy from package.json, but calculated versions including dependencies requirmenets? It could be recomputed only when packages are installed or updated.

Then, later, without changing the lockfile or node_modules folder, you change your node install to v12

That is exactly the case why i want this feature. We are using CI/CD and want to be sure that it's configured properly (with correct node.js version), because we are using some modern features and better to understand if environment is valid on build stage rather than on running stage :)

@markablov
Copy link
Author

@ljharb @isaacs hey, i saw your conversation about my PR at past RFC meeting and just want to make it clear.

I don't suggest to enable --engine-strict by default. But i just want to be sure that if it's enabled, then npm should fail if environment does not satisfy some of requirments of packages. At the same time lockfile should not be ignored, because it's benifitial to control versions of packages (obviously).

@ljharb
Copy link
Contributor

ljharb commented Sep 3, 2020

@markablov My position is effectively:

  1. when engine-strict is enabled, installs should fail whenever engines mismatch.
  2. when it's not enabled (which should be the default), any case where engine-strict would fail should clearly warn in the output, without failing the install/ls.
  3. When the lockfile is formed on node/npm version X, and i switch to version Y, npm ls/npm install should follow the same behavior as if i'd initially created the lockfile at the same day/time, but with version Y instead of X.

@markablov
Copy link
Author

markablov commented Sep 3, 2020

@ljharb so for your 3rd point - if you switch to version Y, and on this version existing lockfile can't be created (because npm install w/o lockfile would fail), then npm ls / npm install should return an error?

I see that your position is a bit wider, because to follow your 3rd point it would be necessary to bake related options (at least engines-strict flag) into lockfile. My request is for simplier check :)

@ljharb
Copy link
Contributor

ljharb commented Sep 3, 2020

Yes, I'd expect that. A lockfile built for node 12 that can't be built for node 14 should not be usable on node 14.

@isaacs
Copy link
Contributor

isaacs commented Sep 17, 2020

Implementation issue: npm/arborist#134

@darcyclarke darcyclarke removed the Agenda will be discussed at the Open RFC call label Sep 18, 2020
@darcyclarke darcyclarke added this to the OSS - Sprint 15 milestone Sep 18, 2020
@darcyclarke darcyclarke self-assigned this Sep 18, 2020
@bonkydog bonkydog self-assigned this Sep 28, 2020
@bonkydog
Copy link

There's still a PR I need to merge on CLI, a bugfix for platform error message generation. Will try to get this in today.

@darcyclarke
Copy link
Contributor

This update went out in 7.0.0-beta.13

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:major backwards-incompatible breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants