Skip to content

update Node.js version 16.x, 18.x, 19.x and add 20.x - #5190

Closed
sheplu wants to merge 1 commit into
expressjs:masterfrom
sheplu:matrix-nodejs-20
Closed

update Node.js version 16.x, 18.x, 19.x and add 20.x#5190
sheplu wants to merge 1 commit into
expressjs:masterfrom
sheplu:matrix-nodejs-20

Conversation

@sheplu

@sheplusheplu commented May 10, 2023

Copy link
Copy Markdown
Member

Update Matrix with Node.js version

  • update v16
  • update v18
  • update v19
  • add v20
  • add v21

@sheplu
shepluforce-pushed the matrix-nodejs-20 branch from f7e9bd3 to 7b59a5aCompareMay 10, 2023 21:42
@sheplu
sheplu marked this pull request as ready for review May 10, 2023 21:48

@rubiinrubiin left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

update to the latest lts which is 18.17.1 as it fixes some CVEs

@rubiinrubiin left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks good. Also I was wondering why are we running tests for all available node versions. Most of them (below 14) have stopped receiving any kind of security updates and its most likely anyone is using them.
Removal of the prior deprecated versions would help speeding up the CI

@sheplu
shepluforce-pushed the matrix-nodejs-20 branch 3 times, most recently from 85a1ff2 to 8dc9cd0CompareAugust 17, 2023 15:53
@sheplu
sheplu requested a review from dougwilsonAugust 17, 2023 15:57
@sheplu
shepluforce-pushed the matrix-nodejs-20 branch 2 times, most recently from 1d27450 to b282698CompareOctober 11, 2023 07:03
@sheplu

Copy link
Copy Markdown
MemberAuthor

@dougwilson I can't manually retrigger all the tests but it should be working fine for all versions

This was referenced Nov 11, 2023

@UlisesGasconUlisesGascon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Seems like the CI issue can be resolved by re-running the CI

@aravindvnair99aravindvnair99 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Something to consider - Do we really need to hardcode the minor values?

@dougwilson

dougwilson commented Nov 11, 2023

Copy link
Copy Markdown
Contributor

Something to consider - Do we really need to hardcode the minor values?

We originally didn't have them, but Node.js would keep releassing versions that would break our CI in the minor. Then CITGM was made to fix that, but I think they removed express so probably even more useful to keep them pinned still.

It really stinks when ever people try and make PRs or we try to do releases when the CI is suddenly broken and then the time needs to be spent fixing that instead.

I think most recent the CI broke in a minor 18.x release of Node.js bc they major bumped npm. Thankfully didn't break CI when that happened so could fix when the focus was on updating CI instead of on something else.

It does have the added benefit too that bumping the minor forces a retest on that new Node.js, otherwise it will sit there until we change something unrelated.

@sheplu

Copy link
Copy Markdown
MemberAuthor

Closing as this will land part of #5429 and #5430

@sheplusheplu closed this Feb 24, 2024
@sheplu
sheplu deleted the matrix-nodejs-20 branch February 24, 2024 14:01
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@sheplu@dougwilson@UlisesGascon@rubiin@aravindvnair99