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: devEngines #7766

Open
wants to merge 19 commits into
base: latest
Choose a base branch
from
Open

feat: devEngines #7766

wants to merge 19 commits into from

Conversation

reggi
Copy link
Contributor

@reggi reggi commented Sep 5, 2024

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
Copy link
Contributor

ljharb commented Sep 5, 2024

What happens without --force?

@reggi
Copy link
Contributor Author

reggi commented Sep 5, 2024

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

@ljharb
Copy link
Contributor

ljharb commented Sep 5, 2024

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
Copy link
Contributor Author

reggi commented Sep 5, 2024

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
Copy link
Contributor

ljharb commented Sep 5, 2024

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
Copy link
Contributor

ljharb commented Sep 5, 2024

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
Copy link
Contributor Author

reggi commented Sep 5, 2024

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
Copy link
Contributor

ljharb commented Sep 5, 2024

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?

package.json Outdated Show resolved Hide resolved
@reggi
Copy link
Contributor Author

reggi commented Sep 5, 2024

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
Copy link
Contributor

ljharb commented Sep 5, 2024

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
Copy link
Member

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

@ljharb
Copy link
Contributor

ljharb commented Sep 5, 2024

@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
Copy link
Member

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
Copy link
Contributor

ljharb commented Sep 5, 2024

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
Copy link
Member

wraithgar commented Sep 5, 2024

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
Copy link
Contributor

ljharb commented Sep 5, 2024

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
Copy link
Member

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

PRs welcome.

@ljharb
Copy link
Contributor

ljharb commented Sep 5, 2024

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

@reggi reggi marked this pull request as ready for review September 6, 2024 16:41
@reggi reggi requested a review from a team as a code owner September 6, 2024 16:41
@hashtagchris
Copy link
Contributor

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
Copy link
Contributor Author

reggi commented Sep 9, 2024

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
Copy link
Contributor

hashtagchris commented Sep 10, 2024

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
Copy link
Contributor Author

reggi commented Sep 11, 2024

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
Copy link
Contributor

ljharb commented Sep 11, 2024

@reggi I agree that devEngines mustn't apply in CI - but since it must apply locally, there must be a way to ignore devEngines (or, union with engines) in CI in order to achieve either goal.

@reggi reggi closed this Sep 11, 2024
@reggi reggi reopened this Sep 11, 2024
@reggi
Copy link
Contributor Author

reggi commented Sep 11, 2024

@ljharb

@reggi I agree that devEngines mustn't apply in CI - but since it must apply locally, there must be a way to ignore devEngines (or, union with engines) in CI in order to achieve either goal.

I never said devEngines mustn't apply in CI. Running package scripts in CI is the same as you running them locally. I can't imagine people remembering to add a --ignore-dev-engines flag, it's an extra step I don't think is necessary. Just don't limit your devEngines to the point ci would fail.

@ljharb
Copy link
Contributor

ljharb commented Sep 11, 2024

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
Copy link
Contributor Author

reggi commented Sep 11, 2024

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
Copy link
Contributor

ljharb commented Sep 11, 2024

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.

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.

4 participants