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

Remove engines property from package.json #91

Closed
wants to merge 1 commit into from

Conversation

kemitchell
Copy link
Member

See #90 (comment)

Per npm's package.json doc, at most setting engines gets us a warning on install. The engineStrict property, which I don't think this package ever set, has been deprecated.

My general thinking on version support so far has been roughly:

  • Checking licenses on long-dormant and "legacy" projects remains important.

  • Many of those projects are locked to old Node.js versions.

  • This is a relatively simple package, without need for many new language or core-library features, anyway.

  • That said, we want to reuse, not reinvent, npm's ways of interpreting and analyzing node_modules directories.

  • npm has its own preferences and constraints for Node.js version support, which we're going to inherit.

Upshot: CI across LTS versions so we can catch and revert new syntax, core APIs, or dependency versions that needlessly sacrifice support for projects on old LTS versions.

I don't like the idea of having to twiddle engines for every release. And I'm not aware of any tools for analyzing our full production dep tree and calculating a composite supported engines range. In practice, we're just having users do that when they run npm install on whatever node and npm they have.

However, in practice, I suspect most projects adopting licensee from scratch will be on later Node.js versions, and get away with just installing the latest licensee. Those diving into old projects will likely have the package.json from days of yore, specifying an old licensee version that worked with whatever Node.js they were using at the time. So long as we and all our dep authors make major version bumps for engine breaks, that should be fine. Alternatively, we could ship npm-shrinkwrap.json, but that makes perpetual bump chores, and we've done fine without it so far.

@ericcornelissen
Copy link
Contributor

Per npm's package.json doc, at most setting engines gets us a warning on install. The engineStrict property, which I don't think this package ever set, has been deprecated.

I might be misreading but I feel like there's some misunderstanding here. The engineStrict property in package.json has indeed been deprecated, but the engine-strict option still exists. The latter can be used by end-users of this (or other) package to ensure they don't run things that don't support their current Node.js version, getting an error otherwise (unless --force is used).

That being said, I think "npm has its own preferences and constraints for Node.js version support, which we're going to inherit." is an appropriate decision for this project.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This is definitely a bad idea. engines defines the API that's supported - if it's not present, then all versions of node are supported, and breaking any of them requires a semver-major change.

@kemitchell
Copy link
Member Author

The tentative plan was to release without engines with a major-version bump. In practice, installs on Node versions not supported by our deps would get warnings. npm install runs with engine-strict would fail.

Interpreting engines: undefined as "all Node versions supported" breaks down for the vast majority of packages that don't set engines, as soon as underlying Node.js changes break them.

Node.js' APIs are over us, not under us. Users will "select" a Node.js version before running npm i. As for compatibility, I don't think this package has anything more to say about Node.js version compatibility than the composite of its deps' constraints.

@ljharb
Copy link
Member

ljharb commented Oct 30, 2023

It doesn't break down at all, * is the explicit default inside npm.

Every package should declare explicit engines, with zero exceptions, and those versions should be what's tested in CI.

I'm really confused what's motivating this PR anyways - #90 made all the right changes for the breaking change it's proposing. What's the benefit of removing engines?

@kemitchell
Copy link
Member Author

Per @ljharb here, this was clearly a misfire. Closing.

As a side note, you're right about the default for engines, @ljharb. But if we interpret what that means strictly---and make it a part of SemVer---then I'm guessing the overwhelming majority of actively used library packages have bad metadata and violate SemVer.

I do like the way Licensee has approached Node version support so far. I don't see any more reason to touch engines. I'd recommend the same approach for other packages with special concern for older compat. But I wouldn't push too hard for everyone else. That way lies...disappointment.

@kemitchell kemitchell closed this Oct 30, 2023
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

3 participants