express icon indicating copy to clipboard operation
express copied to clipboard

Adopt Node@18 as the minimum supported version

Open UlisesGascon opened this issue 1 month ago • 7 comments

As discussed in https://github.com/expressjs/discussions/issues/224 and https://github.com/expressjs/discussions/issues/196.

Here is a proposal to set Node@18 as the minimum version supported by Express v5.x

Main Changes

  • Set Node 18 as the minimum version in the package.json (4b3b8cc231381fe9357d7d98f6afd3e8e9f9d63c)
  • Remove from the CI any Node.js version prior to 18 (e9bcdd399b244079f4cf77dd5ffa58c5831b8b90)

Changelog

  • e9bcdd399b244079f4cf77dd5ffa58c5831b8b90 ci: adopt Node@18 as the minimum supported version by @UlisesGascon
  • 4b3b8cc231381fe9357d7d98f6afd3e8e9f9d63c feat: adopt Node@18 as the minimum supported version by @UlisesGascon

UlisesGascon avatar Apr 11 '24 17:04 UlisesGascon

I had been thinking about this last night after our meeting and I was thinking that we should get all the dependencies working as we expect first and then do this specific change last. Maybe I am over thinking it, but I really do think it will be valuable to point specifically to the commits we landed which "broke" each node.js version. If we stop testing and do this early we loose that signal and it might be hard to untangle after the fact.

wesleytodd avatar Apr 11 '24 17:04 wesleytodd

I had been thinking about this last night after our meeting and I was thinking that we should get all the dependencies working as we expect first and then do this specific change last.

AFAIK, there is no rush to merge this PR. It can be the last one before we officially release v5. Let's keep it open for a while and see how do we feel about it in few more commits :+1:

UlisesGascon avatar Apr 11 '24 17:04 UlisesGascon

Yes and no. Keeping old versions in CI prevents usage of any new JS and Node.js features.

wojtekmaj avatar Apr 11 '24 19:04 wojtekmaj

We can remove them in an ad-hoc way. That is sort of why I am not sure this specific PR would be what we even land, but I don't see any reason not to keep it open for now until we are sure.

wesleytodd avatar Apr 11 '24 19:04 wesleytodd

I had been thinking about this last night after our meeting and I was thinking that we should get all the dependencies working as we expect first and then do this specific change last. Maybe I am over thinking it, but I really do think it will be valuable to point specifically to the commits we landed which "broke" each node.js version. If we stop testing and do this early we loose that signal and it might be hard to untangle after the fact.

If there isnt intent to merge rn then lets move to draft

jonchurch avatar May 04 '24 20:05 jonchurch

So I think maybe the docs have gotten a bit ahead of ourselves here as we say 5.x requires Node 18+ ... https://expressjs.com/en/5x/api.html

IIUC I think we do SUPPORT v18+ but technically we require a lower version. I probably changed this wording because I thought this was decided, my bad.

Probably not a huge deal since anyone who's using 5 has sorted this out... but if this isn't going to land soon, then should we say something different to be accurate?

crandmck avatar May 06 '24 05:05 crandmck

@crandmck you are right, the decision was made already (so in terms of documentation we are fine saying Node@18). This PR has moved to draft as we probably will merge it as the last one before releasing Express@5 ref

UlisesGascon avatar May 06 '24 06:05 UlisesGascon