feat: devEngines

[reggi]

This PR adds a check for devEngines in the current projects package.json as defined in the spec here:
openjs-foundation/package-metadata-interoperability-collab-space#15

This PR utilizes a checkDevEngines function defined within npm-install-checks open here:
npm/npm-install-checks#116

The goal of this pr is to have a check for specific npm commands install, and run consult the devEngines property before execution and check if the current system / environment. For npm the runtime will always be node and the packageManager will always be npm, if a project is defined as not those two envs and it's required we'll throw.

Note the current engines property is checked when you install your dependencies. Each packages engines are checked with your environment. However, devEngines operates on commands for maintainers of a package, service, project when install and run commands are executed and is meant to enforce / guide maintainers to all be using the same engine / env and or versions.

[ljharb]

What happens without --force?

[reggi]

Hey @ljharb, it works as it should. Is this in reference to the description or something you saw in the code?

[ljharb]

No, just from reading your OP - the implication is that npm install will fail with a mismatched devEngines, and that --force is the only way to bypass that, but --force is generally treated as a dangerous command, so it might be nice to have an alternative way to change the default behavior from failure to just a warning?

[reggi]

No, just from reading your OP - the implication is that npm install will fail with a mismatched devEngines, and that --force is the only way to bypass that, but --force is generally treated as a dangerous command, so it might be nice to have an alternative way to change the default behavior from failure to just a warning?

As per the spec it fails if onFail is undefined or set to error and --force is the only way to bypass it.

[ljharb]

ah ok, so if i don't want to use --force, i'd just set onFail to warn in package.json? that works for me :-)

[ljharb]

One additional consideration - only in CI, you'd explicitly need it to not fail (because devEngines might say "node 22" but engines might say "node 18, 20, or 22", and you'd want to be testing on all 3 majors). How could I set that only in CI, if there's no env var or npmrc config to do that?

[reggi]

One additional consideration - only in CI, you'd explicitly need it to not fail (because devEngines might say "node 22" but engines might say "node 18, 20, or 22", and you'd want to be testing on all 3 majors). How could I set that only in CI, if there's no env var or npmrc config to do that?

If you support node 18, 20, or 22 then devEngines should reflect that with a proper semver range.

[ljharb]

That's not the intended usage of the field, though - otherwise engines and devEngines must always be the same, and what's the point of the feature?

[reggi]

That's not the intended usage of the field, though - otherwise engines and devEngines must always be the same, and what's the point of the feature?

I updated the description with some clarifications on engines v devEngines

[ljharb]

Your OP looks great! What I was trying to convey is that, for typical and desired CI usage, there needs to be an npm config-based mechanism to change "fail" to "warn" or "none" (or, to cause it to look at the union of engines + devEngines instead and THEN fail - which might be even better!)

[wraithgar]

If you are testing something in CI why are you setting devEngines outside the bounds of where you test it?

[ljharb]

@wraithgar for a package, CI is for testing what engines matches - the thing consumers will see. devEngines is what contributors should be running locally, which should always be a non-strict subset of engines.

[wraithgar]

Makes sense. That's a whole larger discussion in npm itself in "how do I interact with this code base as if it were a package being used elsewhere". I don't think it's a blocker for this feature.

[ljharb]

It's a blocker for its use in packages, but certainly not one for its use in applications.

It would be very nice for the initial implementation to have an answer for this, though - and my suggestion is either an npm config (and thus an env var) that can override "fail" to "warn", and/or, an npm config (and thus an env var) for checking the union of engines and devEngines before failing/warning/etc.

[wraithgar]

That's a much bigger feature than you think. It effectively means "npm please interact with this top level package.json as if it were a package you are installing" which means parsing engines, os, etc differently. If this is something you need to do then you are much better off doing something like npm pack and then installing that tarball into a separate folder with its own package.json.

Tacking on a "ok just ignore devEngines" flag masks the reality of this disconnect.

ETA: Even the lifecycle scripts act differently when you are installing as a package. Again this is a much bigger issue than just devEngines.

[ljharb]

Fair that it may be large enough of a change to need doing in a separate effort - but "pack in CI and test the output of the tarball" is wildly impractical and incredible difficult to achieve (because of how tests are written), so i don't think that's a viable alternative.

npm already parses engines in your current project, though, and due to workspaces and overrides, it already does something different when it's a project vs a dependency, so I'm not sure why it would be that much larger a feature.

[wraithgar]

so I'm not sure why it would be that much larger a feature

PRs welcome.

[ljharb]

Great, after this lands I'll submit one :-)

[hashtagchris]

Possibly related to @ljharb's comments, what kind of devEngines will we add to npm/node-semver, and how would we change the CI? Or will we intentionally not add one for the time being?

[reggi]

Possibly related to @ljharb's comments, what kind of devEngines will we add to npm/node-semver, and how would we change the CI? Or will we intentionally not add one for the time being?

Because we're still operating on a widely popular runtime / packageManager I don't see much value in adding it, but if we were to add devEngines to npm-semver it would need to match what we have in CI. Something like this:

{
  "devEngines": {
    "runtime": {
      "name": "node",
      "version": ">10.0.0",
      "onFail": "error"
    },
    "packageManager": {
      "name": "npm",
      "version": ">10.0.0",
      "onFail": "error"
    }
  }
}

[hashtagchris]

Thanks @reggi. In practice I think you'd need a newer version of node for development tasks like running lint and postlint. But maybe fully ensuring package developers are set up for success (#7766 (comment)) is a larger feature, like @wraithgar said.

[reggi]

Some thoughts on devEngines in CI:

devEngines was created to enforce mainly packageManager (and runtime) locally. Having just the name specified is a huge help in defining what tools a JS codebase should be developed with. Having version assigned along with the name is simply viewed as another limiting factor. Unlike engines, where the version is a property value of a key and is a requirement, version in devEngines is not required and is viewed as extra.

In CI, codebases support old versions of Node.js. For example, node-semver's npm test script should be runnable and pass in an old version of Node.js, such as version 10. However, npm run lint should be running a modern version of Node.js. It's not within the scope of the devEngines spec to assign specific versions of Node.js for each script in package.json. Again, the job of devEngines is to enforce that the project should be run using npm with packageManager and node. In the case of node-semver, we'd specify npm and node with no version, or have a wide semver range for node so that CI will run.

[ljharb]

As I explained upthread, the only reason for devEngines to exist is so it can be a subset of engines - iow, if the only path forward is to no limit it to the point CI would fail, then the entire feature isn't worth adding, because engines already does that.

npm already does some things automatically in CI when certain env vars are present; it seems pretty reasonable to me to default that flag to true only in CI?

[reggi]

The point of devEngines is for people who use yarn, or bun to enforce that tooling in their project, it seems as an alternative to or additional definition for corepack.

The point of engines is to warn when a dependency is not aligned with the project it's being used in.

then the entire feature isn't worth adding

Maybe it's not for you, if you're just gonna use node / npm.

engines already does that.

engines doesn't throw if there's a mismatch locally, it only warns. engines doesn't cause npm to fail if yarn is specified which there isn't even valid consideration for.

I think engines and devEngines are quite different in purpose, object shape, I don't think devEngines is even an apt descriptor and causes more confusion.

[ljharb]

It's true that devEngines has more than just runtime/package manager, and it's definitely true that engines doesn't fail, but as part of the group that designed this feature in the first place, I can assure you that the explicit intention of its design is to have engines be expansive, and devEngines (or whatever it's called) be a subset.

[hashtagchris]

Looks good. Following up on a few minor things before I approve.

[hashtagchris]

[hashtagchris]

LGTM. Left some minor non-blocking feedback

[wraithgar]

Re-approving to get things moving since the last feedback was "nonblocking"

[styfle]

onFail can be warn, error, or ignore, and if left undefined is of the same value as error.

What happens when "onFail": "download"? Are there plans to implement that?

[wraithgar]

onFail can be warn, error, or ignore, and if left undefined is of the same value as error.

What happens when "onFail": "download"? Are there plans to implement that?

npm treats that as error.

https://github.com/npm/npm-install-checks/blob/4751cb40207cf48ac21ac041b0c239e3c10d37e0/lib/dev-engines.js#L124-L126

There are no plans for npm to automatically download via this spec.