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

Bumps Node requirements from 14 to 18 #5445

Merged
merged 16 commits into from
Jun 24, 2023
Merged

Bumps Node requirements from 14 to 18 #5445

merged 16 commits into from
Jun 24, 2023

Conversation

arcanis
Copy link
Member

@arcanis arcanis commented May 16, 2023

What's the problem this PR addresses?

We're still supporting Node 14, but it has reached end of life. Node 16 is still maintained, but will reach an early end of life in October, so I think it's reasonable to drop it now rather than publish a major release just for that.

How did you fix it?

Bumps the requirements from 14.16 to 18.12 (first LTS from the 18.x release line).

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@merceyz merceyz self-requested a review May 16, 2023 09:15
@arcanis arcanis mentioned this pull request May 16, 2023
3 tasks
Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

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

I don't see the need of dropping support for these Node.js versions as it doesn't cost us much to keep support for them.
If that changes we can drop them and release a new major version at any point.

rather than publish a major release just for that

It's just a number.

@arcanis
Copy link
Member Author

arcanis commented May 16, 2023

The thing is, we didn't treat it as just a number so far, and I think it's more than that in the mind of (at least a part of) the community. There's an argument to be made to change that, but it has some ramifications and I'd prefer not to rush it and repeat the kind of misunderstanding from the v2 release (in other words, that seems a post-4.0 discussion to me).

As for upgrading Node, I'm fine with keeping a loose requirement on the CLI check, but I'd prefer to avoid keeping an outdated transpilation for the sake of Node 14 users. Node 16 is perhaps another story since it has a few more months, but considering it'll quickly become unsafe (due to the OpenSSL release line falling out of support, causing Node to do the same), it could even be seen as encouraging a good practice.

@arcanis
Copy link
Member Author

arcanis commented May 23, 2023

I removed the reverts on logic updates, but kept the test runtime upgrade & engines field upgrade.

@merceyz
Copy link
Member

merceyz commented May 23, 2023

If you absolutely want to drop support for Node.js <18 we should do it fully, not half-way as that's confusing.

I don't want to prevent projects from upgrading Yarn when we don't gain much, if anything, by dropping support for Node.js versions they still support.
For example Babel, depending on the outcome of babel/babel#15585.

@nicolo-ribaudo
Copy link
Contributor

For example Babel, depending on the outcome of babel/babel#15585.

Note that we first run Yarn and then change the Node.js version, so this wouldn't affect us :) We currently use Yarn 3 even if Babel still supports Node.js 6.

@arcanis
Copy link
Member Author

arcanis commented Jun 7, 2023

If you absolutely want to drop support for Node.js <18 we should do it fully, not half-way as that's confusing.

Let's do it then; since Yarn is versioned by project, it shouldn't break setups outside of upgrade attempts, which are anyone subject to other breaking changes. Worst case, if someone has a very valid use case, we'll be able to revert it without having to release a new major. I'll add back the original changes.

Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

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

A few more files needs to be updated, mainly the rollup.config.js files, see the list from #4196 (review).

CHANGELOG.md Outdated Show resolved Hide resolved
@arcanis arcanis requested a review from merceyz June 20, 2023 18:46
Comment on lines 4 to 7
export const HAS_UNFLAGGED_JSON_MODULES = major > 17 || (major === 17 && minor >= 5);

// JSON modules requires import assertions after https://github.com/nodejs/node/pull/40250
export const HAS_JSON_IMPORT_ASSERTION_REQUIREMENT = major > 17 || (major === 17 && minor >= 1) || (major === 16 && minor > 14);
export const HAS_JSON_IMPORT_ASSERTION_REQUIREMENT = major > 17 || (major === 17 && minor >= 1);
Copy link
Member

Choose a reason for hiding this comment

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

Should be able to remove these flags since they'll always be true.

@arcanis arcanis requested a review from merceyz June 22, 2023 15:00
merceyz
merceyz previously approved these changes Jun 22, 2023
Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

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

LGTM, though there is a merge conflict that needs to be resolved.

@merceyz
Copy link
Member

merceyz commented Jun 22, 2023

Internal Error: spawn ETXTBSY
https://github.com/yarnpkg/berry/actions/runs/5350638693/jobs/9703540026?pr=5445#step:5:1067

Should be fixed by libuv/libuv#4059 but that isn't released yet so we can either disable io_uring support by setting UV_USE_IO_URING=0 in the environment or lock the CI to Node.js v20.2.0 until the fix is out.
I vote for the latter.

@arcanis arcanis merged commit a006471 into master Jun 24, 2023
3 checks passed
@arcanis arcanis deleted the mael/bump-node-version branch June 24, 2023 06:13
@arcanis
Copy link
Member Author

arcanis commented Jun 24, 2023

Branch protection rules updated to the new job names

arcanis pushed a commit that referenced this pull request Jul 2, 2023
**What's the problem this PR addresses?**

Whenever we change the supported Node.js version we have to manually
update the build target in multiple files.

Ref
#5445 (review)

**How did you fix it?**

Automate it by computing the Node.js version to target for the builds
with `semver.minVersion` and `engines.node`.

**Checklist**
- [x] I have read the [Contributing
Guide](https://yarnpkg.com/advanced/contributing).
- [x] I have set the packages that need to be released for my changes to
be effective.
- [x] I will check that all automated PR checks pass before the PR gets
reviewed.
arcanis pushed a commit that referenced this pull request Jul 2, 2023
**What's the problem this PR addresses?**

After #5445 we can target a newer
ECMAScript version.

**How did you fix it?**

Update the ECMAScript version target.

**Checklist**
- [x] I have read the [Contributing
Guide](https://yarnpkg.com/advanced/contributing).
- [x] I have set the packages that need to be released for my changes to
be effective.
- [x] I will check that all automated PR checks pass before the PR gets
reviewed.
arcanis pushed a commit to yarnpkg/example-repo-zipn that referenced this pull request Jul 3, 2023
**What's the problem this PR addresses?**

Whenever we change the supported Node.js version we have to manually
update the build target in multiple files.

Ref
yarnpkg/berry#5445 (review)

**How did you fix it?**

Automate it by computing the Node.js version to target for the builds
with `semver.minVersion` and `engines.node`.

**Checklist**
- [x] I have read the [Contributing
Guide](https://yarnpkg.com/advanced/contributing).
- [x] I have set the packages that need to be released for my changes to
be effective.
- [x] I will check that all automated PR checks pass before the PR gets
reviewed.
@merceyz merceyz mentioned this pull request Jul 7, 2023
3 tasks
arcanis pushed a commit that referenced this pull request Jul 9, 2023
**What's the problem this PR addresses?**

Node.js v20.4 has been released which fixes
#5445 (comment)

**How did you fix it?**

Use the latest Node.js v20 version.

**Checklist**
- [x] I have read the [Contributing
Guide](https://yarnpkg.com/advanced/contributing).
- [x] I have set the packages that need to be released for my changes to
be effective.
- [x] I will check that all automated PR checks pass before the PR gets
reviewed.
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.

4 participants