jslicense / licensee.js

check dependency licenses against rules
https://www.npmjs.com/package/licensee
Apache License 2.0
185 stars 23 forks source link

Remove engines property from package.json #91

Closed kemitchell closed 8 months ago

kemitchell commented 8 months ago

See https://github.com/jslicense/licensee.js/pull/90#issuecomment-1784162651

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:

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 commented 8 months ago

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.

kemitchell commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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.