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

Clarify that devEngines is a conceptual subset of engines; solve for CI use #28

Open
ljharb opened this issue Oct 8, 2024 · 2 comments

Comments

@ljharb
Copy link
Member

ljharb commented Oct 8, 2024

For an application, engines is useless, so one would only use devEngines (or only use engines), and the incremental value for devEngines is minimal

For a node npm package, the intended and common usage will be something perhaps like this (noting that "see a warning" may be replaced with "fail with an error" if configured as such):

  • engines.node would be eg ^20 || >= 22, indicating you can use node 20 or 22+
  • devEngines.runtime.node would be eg 22, indicating you should develop the package with node 22
  • when the package is being installed, users should see a warning unless they're on node 20 or 22+
  • when npm install is ran by a developer of the package, users should see a warning unless they're on node 22

This matches npm's current implementation of devEngines, as far as I'm aware.

However, there's an additional critically important scenario: CI.

One should be testing this hypothetical package on node 20 and 22+ (every major, or every minor, to the maintainer's preference). However, tests run npm install as if they were a developer of the package. Thus, CI runs that are on node 20 will see a warning (not that big a deal) or, fail the tests. That defeats the entire purpose of devEngines.

Current possible solutions:

  1. in CI, delete devEngines before the workflow run, and hope it doesn't get accidentally committed before you restore package.json
  2. in CI, overwrite devEngines.runtime.node with engines.node (and similarly for devEngines.packageManager.npm and engines.npm, etc) before the workflow run, and hope it doesn't get accidentally committed before you restore package.json

I'm planning to make a PR to npm to create a config setting (that can be set in .npmrc or by environment variable) that when enabled, will check the union of engines and devEngines (instead of just devEngines). Depending on how the npm team's feedback to that plays out, ofc, we should strive to communicate very clearly that such a mechanism is necessary - and that without such a mechanism, devEngines isn't worth implementing at all.

@GeoffreyBooth
Copy link
Contributor

tests run npm install as if they were a developer of the package

Isn’t this assumption perhaps the root problem? If you want to test how a library works for consumers, you should probably install it and run it as a consumer. So from a new app/project, npm install /path/to/library; https://docs.npmjs.com/cli/v10/configuring-npm/package-json#local-paths. When a library is installed as an app’s dependency, devEngines isn’t involved.

Then you would need to figure out how to run the tests from this context, but that’s its own solvable problem. They could be copied into this new folder and run from there, for example.

@ljharb
Copy link
Member Author

ljharb commented Oct 9, 2024

That's a very reasonable position to hold, but it's not what basically anyone ever does (meaning, testing the tarball contents). devEngines wasn't intended to force the entire ecosystem to change its testing practices, and if that's where it remains, then it won't get adoption for any packages, defeating the whole point.

@ljharb ljharb changed the title Clarify that devEngines is a conceptual superset of engines; solve for CI use Clarify that devEngines is a conceptual subset of engines; solve for CI use Nov 14, 2024
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

No branches or pull requests

2 participants