Adopt Node@18 as the minimum supported version

[UlisesGascon]

As discussed in expressjs/discussions#224 and expressjs/discussions#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 (4b3b8cc)
• Remove from the CI any Node.js version prior to 18 (e9bcdd3)

Changelog

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

[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.

[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.

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 👍

[wesleytodd]

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.

[crandmck]

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?

[UlisesGascon]

@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

[RobinTail]

we probably will merge it as the last one before releasing Express@5 ref

I recommend to do it now, @UlisesGascon .
This PR should be treated as a foundation of 5.x, because it enables using modern syntax, latest dependencies and to remove various compatibility crutches in all other PRs.

[RobinTail]

@wesleytodd && @UlisesGascon , by merging this one now, you can unblock another one immediately:

• Use Array.flat instead of array-flatten #5677

It's marked as required for 5.0 in

• Express 5.0 - last push! discussions#233

UPD: Found one more blocked by this:

• update debug to 4.3.5 #5758

[wesleytodd]

The only thing those engines affect is the consumer of express main package

It can have a ripple effect. For example, I have some packages which currently test with express as a dev dep and support node 16. If we add engines I can no longer install the v5 branch in those tests and will need to major version rev that package to drop those tests or come up with some convoluted way around it.

I am alright if we want to go hard on the gas for this, but I am not alright with leaving this open as a draft. We need to pick a lane and commit. If we think the best thing for everyone is to go hard, we can update the engines. If we are not sure we want to do that, we can drop it for CI only for now until we can get all the packages updated and passing tests which is an easy way to land this without having to discuss more.

[RobinTail]

I have some packages which currently test with express as a dev dep and support node 16.

I see.
Node 16 is EOL, it should not be used anyway — bump the major version of that package and set the min Node to 18 there, @wesleytodd . That will unblock you.

I am alright if we want to go hard on the gas for this

Yes, we want to go hard: untangle ourselves, enable the progress, provide to community, ensure the future.

[ctcpip]

If we add engines I can no longer install the v5 branch in those tests and will need to major version rev that package to drop those tests or come up with some convoluted way around it

why's that? incompatible engines should only produce a warning, unless engine-strict is specified. you should not be blocked from installing

[wesleytodd]

I am just saying it is common to see breakage when folks change engines and it is not always simple or clear how to best proceed. I assumed this was a draft and not already merged out of an abundance of caution. If folks are ready to push forward on this, which I am if y'all are, then lets just do it and merge this now. @UlisesGascon if you are ready for that than can you convert this from draft and then we can approve and merge?

[wesleytodd]

Re-reading the thread I think maybe some wires are crossed. I did not mean to block anything (I would have put a changes requested review on it if I had) with my comment above about lining up the other packages first. It was just a proposal to make sure we didn't accidentally miss a breaking change we needed to call out. At this point I have gone through the changes in all the sub packages I see with obvious pending work and am starting to remove the CI for old node versions, so I am not sure the original reason holds up anymore.

[ctcpip]

I may be mixed up here too. 😅 Re-reading as well:

I really do think it will be valuable to point specifically to the commits we landed which "broke" each node.js version

It was just a proposal to make sure we didn't accidentally miss a breaking change we needed to call out.

Ok, I think I understand your concerns better now. This could be tough. Once we introduce the first change that breaks a version of node, it can be difficult to determine what subsequent changes also break that version of node. But maybe we get around that by not merging anything, and just lining everything up in separate PRs, which is doable as long as there is no blocking/dependent cascade... I am probably overthinking it.

So I agree there is no rush to merge it, and it's possible that the breaking change information could be useful.

[wesleytodd]

Cool, glad my idea made some sense lol. I just want to know when we break it even if it is only the first break since that is all around better than not knowing 😄

EDIT: to clarify a bit on top of what @ctcpip said, the idea I had was that we would be able to tell exactly what commit broke people in this complicated and intertwined web of packages. If we cannot figure out which version broke a specific node version it means we just have a more difficult time tracking it down if necessary. This is not a blocker, but it could be useful in the future. Again, this should not stop us from just landing this change, just context to be aware of. So my earlier statement stands, if this is not a draft anymore and we have the approvals, we should just land it and move forward.

[RobinTail]

we should just land it and move forward

Amazing! When do we start doing that, @wesleytodd ?

[UlisesGascon]

This PR is not a draft anymore, feel free to merge it when you are ready @wesleytodd

[wesleytodd]

Can we change the name of this to legacy? And maybe also the names of the individual stages as well?

[ctcpip]

whoops. missed that

[ctcpip]

does it look ok now?

[wesleytodd]

Great addition! We can still tell when CI fails but it does not block! I only made one comment but then yeah I think we should land this!

[ctcpip]

the change also unpins the node minor versions, something we previously agreed to... and I see that is now causing 21 to fail with our favorite query issue. probably solved with a rebase. checking

[ctcpip]

1. PR to update 5.0 -- https://github.com/expressjs/express/pull/5785/files
2. update/merge that when ready (may need a 4.x release first)
3. rebase this PR on the updated 5.0 branch
4. CI should then pass
5. merge this PR

[UlisesGascon]

based on the CI output, should we skip this tests in 21? Due the known QUERY issue.

[wesleytodd]

This is what @ctcpip was hoping to fix with merging the 4.x branch which has a change to skip that QUERY test. We could also consider cherry-picking that commit over if we want to fix it sooner than the next release of v4.