Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for the new Node LTS (version 20) #2363

Merged
merged 7 commits into from
Nov 3, 2023
Merged

Conversation

BenSurgisonGDS
Copy link
Contributor

@BenSurgisonGDS BenSurgisonGDS commented Oct 20, 2023

As the current LTS version of node.js is 20 and the maintenance version is 18, we should reflect that within the protoype kit.

See here for the current LTS version: https://nodejs.org/en/download

@BenSurgisonGDS BenSurgisonGDS force-pushed the test-node-20 branch 5 times, most recently from a8681db to dab78c4 Compare October 27, 2023 10:17
@BenSurgisonGDS BenSurgisonGDS marked this pull request as ready for review October 27, 2023 15:00
@BenSurgisonGDS BenSurgisonGDS requested a review from a team October 27, 2023 15:00
@BenSurgisonGDS BenSurgisonGDS changed the title Upgrade to node 20 Support for the new Node LTS (version 20) Oct 27, 2023
@@ -43,6 +43,13 @@ describe('plugin-validator', () => {
})

describe('unexpected keys', () => {
afterEach(() => {
// prevent the process exitcode of 100 from failing the tests in the CI
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment describes the code but not the reason. What does exit code 100 mean and why do we need to avoid that failing the CI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see here where the test fails in the CI: https://github.com/alphagov/govuk-prototype-kit/actions/runs/6668446182/job/18124028349

The tests actually pass, but the action fails due to the process completing with exit code 100

The exit code 100 is set in the code and now that we have moved to node 20, the unit tests fail when we don't unset it from 100

The fix in the tests was the only way I could get this to pass.

@nataliecarey
Copy link
Contributor

This PR drops support for 16 which we shouldn't be doing. We should be adding support for 20 without dropping support for 16.

We should also go back to using a specific version in our .nvmrc file, that should be 20.9.0 as that's the current LTS.

@colinrotherham
Copy link
Contributor

Hope this is useful, but you'll probably want Node.js v20.x rather than v20.9.0 or you'll miss new releases:

- lts/*
+ lts/iron

We've just completed our GOV.UK Frontend switch to Node.js 20

Snags wise, Dependabot is still using Node.js 18 which caught us out today:

But otherwise all our tooling blockers were completed and I've added notes for asdf or nvm gotchas

@colinrotherham
Copy link
Contributor

Bear in mind too that you're still on the legacy { "lockfileVersion": 2 } format

Similar to how Node.js 18 shipped with npm v8 but later npm v9, watch out for this to become an issue:

@nataliecarey
Copy link
Contributor

With the version - I think it's important that we specify the version in .nvmrc as it allows reproducibility if we're looking at the same code in the future rather than it using whatever version is lts at that future time.

@colinrotherham
Copy link
Contributor

With the version - I think it's important that we specify the version in .nvmrc as it allows reproducibility if we're looking at the same code in the future rather than it using whatever version is lts at that future time.

That's fair, don't forget your node-version matrix uses the latest (cached) version too

matrix:
  node-version: [16.x, 18.x, 20.x]

package.json Outdated Show resolved Hide resolved
@@ -14,11 +14,13 @@ describe('plugin-validator', () => {

beforeEach(() => {
const mockFileSystemRoot = path.join(process.cwd(), 'example-plugin')
const exitCodeSetterSpy = jest.spyOn(process, 'exitCode', 'set').mockImplementation(() => {})
Copy link
Contributor

Choose a reason for hiding this comment

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

🙌

colinrotherham
colinrotherham previously approved these changes Nov 2, 2023
bin/cli Outdated Show resolved Hide resolved
@BenSurgisonGDS BenSurgisonGDS merged commit a3e4c96 into main Nov 3, 2023
30 checks passed
@BenSurgisonGDS BenSurgisonGDS deleted the test-node-20 branch November 3, 2023 12:42
@36degrees 36degrees mentioned this pull request Feb 22, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants