Skip to content

feat: add limited support for devEngines - #643

Merged
aduh95 merged 21 commits into
nodejs:mainfrom
aduh95:dev-engines-part1
Feb 28, 2025
Merged

feat: add limited support for devEngines#643
aduh95 merged 21 commits into
nodejs:mainfrom
aduh95:dev-engines-part1

Conversation

@aduh95

@aduh95aduh95 commented Feb 8, 2025

Copy link
Copy Markdown
Contributor

This implementation of devEngines support in this PR is limited to packageManager only, only one packageManager can be specified, the onFail field is ignored EDIT: I've added support for the onFail field, supporting the same values as npm.

Fixes: #567

Comment threadsources/specUtils.ts Outdated
Comment threadsources/specUtils.ts Outdated
Comment threadtests/main.test.ts
Comment threadsources/specUtils.ts Outdated
Comment threadtests/main.test.ts
Co-authored-by: Geoffrey Booth <webadmin@geoffreybooth.com>
@styfle

styfle commented Feb 9, 2025

Copy link
Copy Markdown
Member

the onFail field is ignored

What value of onFail is used by default if its not accepting user input? download?

@styflestyfle left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How about updating the readme?

In particular, mentioning the onFail behavior and also which takes precedence when both packageManager and devEngines is defined?

@aduh95
aduh95 requested a review from styfleFebruary 15, 2025 19:04
Comment threadREADME.md
- if set to `warn` or some other value, Corepack will print a warning in case
of mismatch.

If the top-level `packageManager` field is missing, Corepack will use the

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be the first sentence of this section.

Because it sounds like this devEngines.packageManager is ignored when there is a top level packageManager, right?

@aduh95aduh95Feb 25, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

No it's never ignored. As said in the first paragraph of this section, if it is defined to something Corepack recognises, it will throw/warn/do nothing (depending of the value of onFail) if the user is trying to use an incompatible package manager (i.e. if the one defined in package.json#packageManager does not match the one defined in package.json#devEngines.packageManager for when the user is using a Corepack shim)

Comment threadREADME.md

Unlike `corepack use` this command doesn't take a package manager name nor a
version range, as it will always select the latest available version from the
range specified in `devEngines.packageManager.version`, or fallback to the

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What about top-level packageManager?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That's what or fallback to the same major line is for

Comment threadsources/specUtils.ts
Comment threadsources/commands/Base.ts Outdated
Comment threadsources/commands/Base.ts Outdated
Comment threadsources/commands/Base.ts Outdated
@aduh95

This comment was marked as resolved.

@aduh95
aduh95 merged commit b456268 into nodejs:mainFeb 28, 2025
@aduh95
aduh95 deleted the dev-engines-part1 branch February 28, 2025 19:06
@github-actionsgithub-actionsBot mentioned this pull request Feb 28, 2025
haoqunjiang added a commit to haoqunjiang/schemastore that referenced this pull request Mar 13, 2025
madskristensen pushed a commit to SchemaStore/schemastore that referenced this pull request Mar 13, 2025
* Support `devEngines` field in `package.json`
It's a [standardization effort by the OpenJS Foundation](https://github.com/openjs-foundation/package-metadata-interoperability-collab-space/blob/43e31689ae1fff8c46b617548655a3f60e0c9387/devengines-field-proposal.md)
and has been implemented in [NPM 10.9.0](npm/cli#7766)
and [Corepack 0.32.0](nodejs/corepack#643).
* [pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
---------
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Comment threadsources/specUtils.ts
onFail: selection.data.devEngines.packageManager.onFail,
},
// Lazy-loading it so we do not throw errors on commands that do not need valid spec.
getSpec: () => parseSpec(rawPmSpec, path.relative(initialCwd, selection.manifestPath)),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

devEngines allow to specify version ranges - this now causes this to throw an exception if you pass a version like ^10.
IMHO here the enforceExactVersion option of parseSpec needs to be set to false if devEngines is used.

Comment threadtests/main.test.ts
});
});

describe(`should accept range in devEngines only if a specific version is provided`, () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This behavior breaks existing projects which only define a range in devEngines (e.g. to make developers all use same major line of the package manager).
Normally that would be somewhat ok as its only your project and you know what you are doing, but this affects a lot of different tools like dependabot that are now broken for projects that not use corepack themself.

Sign up for freeto 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.

Implement devEngines proposal from Package Metadata Interoperability Collab Space

7 participants

@aduh95@styfle@GeoffreyBooth@arcanis@susnux@MikeMcC399@merdopo66