-
Notifications
You must be signed in to change notification settings - Fork 143
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
@W-14545340@ Add support for node 20 #1612
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes to engines
are missing in -lock.json
files.
Lines 25 to 28 in 23705b4
"engines": { | |
"node": "^16.11.0 || ^18.0.0", | |
"npm": "^8.0.0 || ^9.0.0" | |
} |
Generated with node v18.18.0 and npm 9.8.1.
"node": "^16.0.0 || ^18.0.0", | ||
"npm": "^7.0.0 || ^8.0.0 || ^9.0.0 || ^10.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we define only Node 18 and Node 20 on a generated project, while we define Node 16, Node 18, and Node 20 in monorepo packages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to break existing projects, if we have any customers who are still using node 16 for local development. (Which would be weird, but technically possible.) For newly generated projects, though, we don't have to worry about supporting legacy versions!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I thought that was the case, in theory dropping Node 16 would be a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I smoke-tested the generated template-retail-react-app project using Node 20 locally and things work as expected.
I don't see new warnings or errors when running npm i
locally using Node 20.
Fixes #1618. |
Description
This PR adds support for node 20/npm 10, mostly by just updating our engines to allow it. Note that Managed Runtime does not currently support node 20, so this does not validate deployed apps beyond "can be built with node 20 and executed with node 18".
As discovered in #1535, upgrading to node 20 mitigates one out of memory issue present in node itself. However, after a bit of testing, this did completely mitigate the issue.
Types of Changes
Changes
How to Test-Drive This PR
To test functionality:
To test OOM issue, start the dev server and then execute the following script from
packages/template-retail-react-app
. It updates a file, waits 3 seconds, then makes a request. It repeats until the app crashes or we reach 1000 iterations. Running this locally, I had the app crash after 59 iterations for node 18 and 69 iterations for node 20.Checklists
General
Accessibility Compliance
You must check off all items in one of the follow two lists:
or...
Localization