Discussion: Using engines in the package.json

[UlisesGascon]

Currently some packages has drop support to Node.js prior to v18, seems like we didn't upgrade the engines field in the package.json discordantly.

So, I will like to discuss if this a policy that we should follow as an organization to all the packages (so in that case we can also document it).

⚠️ So far seems like Express actually updated the package (ref) but not in others like finalhandler (ref)

Ref:

• Missing engines Field Update for Node.js Version Support in v2.0.0 pillarjs/finalhandler#64
• fix(package.json) Explicit drop support for node<18 pillarjs/finalhandler#59 (great context on why not update by @wesleytodd )

cc: @expressjs/express-tc @Phillip9587

[UlisesGascon]

IMO, we should align the engines to the CI versions we test for each release branch. Otherwise, we risk introducing breaking changes without realizing it.

If the package happens to work in older versions of Node.js that we haven't explicitly defined as supported, it should be considered an "unsupported feature." We should encourage end users to use at least the minimum supported version for that release branch. Otherwise, we'll end up "supporting" unintended scenarios based on environments we don't explicitly endorse, leading to issues like unexpected bug reports and slower development due to legacy version compatibility problems.

Additionally, npm is clear in its documentation: unless the user has set the engine-strict config flag, the engines field is advisory only and will simply produce warnings when your package is installed as a dependency. This reinforces the need to clearly define supported versions, ensuring that users are aware of the minimum requirements. (documentation)

[ctcpip]

agree, engines should be added where missing, and should match our CI matrix

[Phillip9587]

I completely agree with the proposed solution to establish a consistent policy for updating the engines field across all packages. Ensuring that the Node.js version support is clearly defined and aligned accross the packages makes sense from a maintenance and user experience perspective.

I’m happy to contribute and help move this forward by submitting the necessary PRs to update the relevant packages. Let me know how best to coordinate on this!

[ljharb]

100% that CI should match whatever engines says.

(Changing engines in a way that reduces support is a breaking change, even if the software didn't work on the removed envs, ftr)

[inigomarquinez]

I completely agree with the proposal!

[bjohansebas]

I totally agree on keeping the CI in line with what the engines say

[UlisesGascon]

Proposal for adr created #289

[ctcpip]

TC agreed today:

• use engines field
• engines should reflect minimum supported* node version
  • *supported means what the project has agreed to support, not necessarily the minimum version it can actually run on
• engines should match CI
  • CI may include older unsupported engines, particularly because it's still useful to know which versions still work and when they break
  • for example, express still runs node 16 and 17 in CI and will continue to do so until a change breaks it
• ideally, we do not want to arbitrarily drop node version support for projects. in other words, there should not be a major version rev JUST for an engines change. there should be other changes that necessitated the dropping of old node support, e.g. wanting to use new node or ECMAScript features

This is my understanding from today's discussion; others please correct anything if I've gotten it wrong.

[wesleytodd]

OMG I was so heads down today I totally missed the notification for this. Sorry. I am guessing this one wasn't recorded?

EDIT: I can ask this in slack or find it myself.

---

use engines field

Did y'all see that post from the fastify folks about this? They had removed it because of the pain it caused. I think we can avoid that as long as we really pick the absolute minimum we support in the major line and dont change it at all for the entire major. But just wanted to call it out in case y'all hand't seen it (I can find a link if necessary nice, the link was in that other pr: fastify/fastify#5667).

in other words, there should not be a major version rev JUST for an engines change. there should be other changes that necessitated the dropping of old node support, e.g. wanting to use new node or ECMAScript features

I am 👍 on this.

[ljharb]

It's also worth running tests in both "first" and "latest" of a line - iow, if you support ^18, then CI should at least test ~18.0 as well as 18.

[wesleytodd]

I might have missed it in here, but as we had folks start opening PRs for this in the code and learn yesterday I realized that it wasn't clearly called out that changing the engines field is considered a breaking change. And while we changed it in express if we are going to update them in the sub packages we will be slowly landing those as we land new BREAKING changes. I think that is what y'all meant with " there should be other changes that necessitated the dropping of old node support" but maybe we need to create an ADR with the new format to clarify this?

[UlisesGascon]

Maybe we need to create an ADR with the new format to clarify this?

It is here: #289. I think the major point of the ADR is that the CI should match the engines, and we should include engines where is missing.

So, if we decide to drop support in the CI, it then affects the engines... so that is a major. Adding an engines field to an existing project that aligns with what is supported in the CI is not a major change (IMO) as it essentially reinforces the current state of that version.

That said, I agree that simply bumping Node.js for the sake of bumping Node does not justify a major change, but this might be something to cover in a different ADR related to "when to do a major?" :)

[ljharb]

@UlisesGascon unfortunately adding an engines field to a project is indeed a breaking change, unrelated to what's in CI, iff the package worked on a now-excluded node version.

[Phillip9587]

@ljharb I’ve noticed that many packages list in their HISTORY.md that support for Node.js below v18 has been dropped, but the engines field hasn’t been updated to reflect this. Would updating the engines field to match what’s already stated in HISTORY.md be considered a breaking change? Since the documentation already indicates support has shifted, it seems like this change would simply formalize the current compatibility and not necessarily introduce a breaking change. What do you think?

[voxpelli]

@Phillip9587 Its almost a philosophical question – it’s like bug fixes that are breaking changes but are breaking changes that restore the intended functionality. In the strict way of interpretations they need to be major releases, but some common sense is needed

To me it would make sense to release it as patch – it would be the most correct from the perspective of this at its core being a “bug fix” – ensuring the range is updated in all places.

I believe that @ljharb follows a stricter interpretation than me, and that’s fine, ultimately you need to decide with what you think is the most correct according to SemVer – whether you consider it a bug fix or a new breaking change.

[ljharb]

@Phillip9587 again, yes - adding engines is only nonbreaking if every engine on which it already works is included in the range.

Indeed “works” can be vague/gray, but despite the impossibility of describing a rubric, I’ve almost never found it a difficult decision to make ad hoc.

[voxpelli]

@ljharb It will only break when strict engine checks are used and only when installed on engine versions that might anyhow no longer be compatible with the code.

The older engine versions are only safe if the support is rolled back to that of the engine declaration, the current major is marked as deprecated, encouraging them to downgrade or upgrade to a new major with correct claims or the current major gets patched (and the previous releases of it possibly deprecated)

I typically think this should be a major, but in this case I think it’s to be seen as a bug that people upgrading should have already adapted to

[ljharb]

Many tools check the engines field, and nonzero breakage is breakage, full stop.

I agree that whatever it worked on is what should end up in engines and CI, and then to reduce that, a major bump should occur. Obviously it’s fine to skip the “fix engines/ci to match reality” step as long as engines isn’t released in a non-major.