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

fix: require Node v20.9.0+ [BREAKING CHANGE] #4484

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

nolanlawson
Copy link
Collaborator

@nolanlawson nolanlawson commented Aug 23, 2024

Details

BREAKING CHANGE: require Node v20.9.0+.

Node support calendar for reference.

This also updates our CI tests to test in the earliest version of Node that we can possibly support.

The reason for 20.9.0 in particular is that it's the lowest version that all of our dependencies/devDependencies support.

Does this pull request introduce a breaking change?

  • 😮‍💨 No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

  • 🤞 No, it does not introduce an observable change.

@nolanlawson nolanlawson requested a review from a team as a code owner August 23, 2024 21:55
@nolanlawson nolanlawson added this to the 8.0.0 milestone Aug 23, 2024
package.json Outdated
"volta": {
"node": "20.12.2",
"node": "22.7.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Locally, we may as well use a recent Node version. CI tests can still fail if we use something that doesn't work in Node 18.

Copy link
Contributor

Choose a reason for hiding this comment

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

I usually prefer to use the oldest supported version. We're more likely to introduce code that works in a new version, but not an old one, than we are the reverse, and I don't like waiting for CI for an annoying (and usually trivial) error.

@@ -87,11 +87,8 @@
"packages/lwc",
"playground"
],
"engines": {
"node": ">=10"
},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This config does nothing since the root monorepo package.json is private.

@nolanlawson nolanlawson changed the title fix: require Node v18+ fix: require Node v20+ Aug 23, 2024
Copy link
Contributor

@wjhsf wjhsf left a comment

Choose a reason for hiding this comment

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

Gotta love a good commit history 🧑‍🍳🤌

@@ -24,7 +24,7 @@ jobs:
- name: Setup Node
uses: actions/setup-node@v3
with:
node-version: '20.12.2'
node-version: '20.9.0' # earliest node version we support (via "engines" in package.json)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way we could read this from a file, so that we don't have to maintain it in multiple places? (Or just a single GHA env var...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sadly I could not find a way to do this in GHA.

package.json Outdated
"volta": {
"node": "20.12.2",
"node": "22.7.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

I usually prefer to use the oldest supported version. We're more likely to introduce code that works in a new version, but not an old one, than we are the reverse, and I don't like waiting for CI for an annoying (and usually trivial) error.

@@ -106,6 +106,9 @@ for (const dir of globSync('./packages/@lwc/*')) {
bugs: { url: 'https://github.com/salesforce/lwc/issues' },
license: 'MIT',
publishConfig: { access: 'public' },
engines: {
node: '>=20.9.0',
Copy link
Contributor

Choose a reason for hiding this comment

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

How strict is this version? Does the code fail to execute on 20.8 or earlier or is this just treadmilling?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It will fail to npm install, so it's kind of a blocker for people with older Node versions.

The reason for 20.9.0 is that this limit is set by our devDependencies (I forget which one).

Technically devDeps don't matter for our consumers, but IME it's a huge pain to try to define one minimum Node version for your deps and another for your devDeps.

@nolanlawson nolanlawson changed the title fix: require Node v20+ fix: require Node v20.9.0+ Dec 11, 2024
@nolanlawson nolanlawson changed the title fix: require Node v20.9.0+ fix: require Node v20.9.0+ [BREAKING CHANGE] Dec 11, 2024
@cardoso
Copy link
Contributor

cardoso commented Dec 17, 2024

Perhaps this PR could change the tsconfig moduleResolution to "node16"? The "node" value is actually deprecated and is an alias to "node10". Although I'd probably just change it to "bundler".

"moduleResolution": "node",

@nolanlawson
Copy link
Collaborator Author

@cardoso This PR is about the code we ship to consumers, whereas tsconfig.json should only affect our internal monorepo setup. It looks like we could set it to "bundler" today without shipping any breaking changes – although I do see some TypeScript errors. Separate PR? 🙂

Sign up for free to 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.

4 participants