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: add devEngines field #7253

Closed
wants to merge 1 commit into from
Closed

feat: add devEngines field #7253

wants to merge 1 commit into from

Conversation

ljharb
Copy link
Collaborator

@ljharb ljharb commented Feb 27, 2024

This field is only looked at when in the top-level project, and takes precedence over engines when present. It's particularly useful when you want devs to use a more modern node/npm/etc version than your package actually supports.

An alternative implementation instead of a new top-level field would be publishConfig.engines, which when present/published would override engines for NON top-level projects (this would mean that the engines field is always the source of truth, but the packument is altered on publish when publishConfig.engines is present). Happy to make that change if preferred.

(This would be an RFC, but RFC calls have been dead for years)

This field is only looked at when in the top-level project, and takes precedence over `engines` when present

An alternative implementation instead of a new top-level field would be `publishConfig.engines`, which when present would override `engines` for NON top-level projects.
@darcyclarke
Copy link
Contributor

darcyclarke commented Feb 27, 2024

As I mentioned to @ljharb in a side channel, I think this feature could/should land as publishConfig.engines although its likely a bit more work.

Why publishConfig.engines & not devEngines?

  • root-level package.json engines.npm definitions already throw warnings/errors for Arborist cmds (ie. install, update, audit etc.) - there's no need for two fields doing the same thing (ie. what is the expected/supported npm or node version for this package OR project)
  • engines definitions that are mismatched with the npmVersion &/or nodeVersion should always warn end-users (this can be hidden with loglevels if undesirable)
  • engines already has usage (~1.3M)
  • creating a new, development-specific field won't ensure its left out of published packages or ignored by other tools (ex. packageManager has begun to creep into package distros - ex. include: typescript, @swc/core, chart.js, devalue & many more)
  • publishConfig definitions are specifically intended to override project/development config in place of package/consumer values (it seems like the right prior-art to work off of)
  • adding engines as a config aligns with how os & cpu are treated as both configs & package.json key/values

Why not publishConfig.engines?

the devils advocate...

  • you'll need to manipulate the package's package.json in-memory during pack/publish (which has been prone to error historically, although other package managers successfully do this)
    • this may not be a deal-breaker since you'd likely want to gate &/or remove any alternative references during pack/publish if you chose a different key (ex. devEngines)

How to implement publishConfig.engines?

  • @ljharb has a good base here although there's several things missing...
  • add a consistent check for project-level engines.npm & warn accordingly (ref. https://github.com/npm/cli/blob/latest/lib/es6/validate-engines.js) - this may be semver-major so it could come later or behind a flag (ex. --validate-engines-npm)
  • add a set of new configs: engines-* that inherit their values from the corresponding root-level package.json key/values by default (ie. engines.*)
    • this should follow the existing @npmcli/config algo which walks up the path & reads package.json along the way to the root (ref. https://github.com/npm/cli/blob/latest/workspaces/config/lib/index.js#L702)
    • there's likely an optimization that could be made here to not throw away the contents of the root package.json & cache it in the loaded config for use with commands like run, pkg, view etc.
  • add support for publishConfig.engines.* definitions
    • these will override the root-level config & values prior to packing
    • if no value is found & no config defined then the default should be undefined/null (this way you don't start accidentally adding engines definitions when publishing packages by default - aka. a breaking change)
    • os & cpu should be considered as other fields which could be supported (given that these are often used during conditional installation but may want different values during development)
  • npm pack should support publishConfig &/or npm publish --dry-run should be able to output a tgz with a similar --pack-destination flag as pack
    • this would help when debugging the contents of the package to ensure the package.json was modified with the right engines values based on all the config

@ljharb
Copy link
Collaborator Author

ljharb commented Feb 27, 2024

I don't think it makes sense to hardcode just node and npm - there's lots of things that can go in engines, and all of them should be supported whether in devEngines or publishConfig.engines.

@darcyclarke
Copy link
Contributor

darcyclarke commented Feb 27, 2024

Makes sense, engines-* it is although npm should only really care about validating npm & node. Since that check isn't happening today, & would be a breaking change to add it, maybe it can be thrown into a separate PR (or shipped behind a flag) while the net-new config & publishConfig.engines.* shenanigans can likely be added as semver-minor.

@GeoffreyBooth
Copy link

If this field could be made equivalent with Corepack’s "packageManager" field, we could suggest that Corepack switch to using this field instead. Then the three major package managers would all respect the same field that denotes desired package manager and version.

What this would need for equivalence is support for URLs (to download the package manager version) and hashes (to validate the download). See:

"packageManager": "yarn@3.2.3+sha224.953c8233f7a92884eee2de69a1b92d1f2ec1655e66d08071ba9a02fa"

or

"packageManager": "yarn@https://registry.npmjs.org/@yarnpkg/cli-dist/-/cli-dist-3.2.3.tgz#sha224.16a0797d1710d1fb7ec40ab5c3801b68370a612a9b66ba117ad9924b"

Obviously npm would have no use for these, but there would need to be a place to define these values in devEngines so that Corepack could use them. If there was, then Corepack could migrate to using devEngines with no loss of functionality on Corepack’s side.

@ljharb
Copy link
Collaborator Author

ljharb commented Feb 27, 2024

I don’t think that expansion of the field makes sense; that functionality in corepack is both recent and ill-advised and i wouldn’t want to see it replicated anywhere else.

This field is intended to only be the dev-time mirror of the engines field (or, in the alternative explanation, the publish-time mirror).

@wraithgar
Copy link
Member

In order to not shard our design efforts it is probably worthwhile to keep discussion in nodejs/node#51888 for now. I like the idea of having a discrete "interacting with this as an app" directive for npm that is different than "interacting with this as a published package". I don't know that a net new top level attribute for this single function is the right approach. We're heading in the right direction though. See you in the other issue.

I'm closing this not as a hard "no" but (hopefully) in service of consolidating design concerns.

@wraithgar wraithgar closed this Feb 27, 2024
@ljharb
Copy link
Collaborator Author

ljharb commented Feb 27, 2024

@wraithgar I don't think it's a good idea to delay this long-discussed and much-needed feature indefinitely until something larger and more complex is designed and widely shipped .

If you don't want a top-level attribute, what do you think about the publishConfig.engines approach suggested by @darcyclarke?

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.

None yet

4 participants