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

feat!: Require Node.js ^18.18.0 || ^20.9.0 || >=21.1.0 #17725

Merged
merged 4 commits into from
Dec 20, 2023
Merged

Conversation

mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Nov 6, 2023

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[x] Add something to the core
[ ] Other, please explain:

Fixes #17595

What changes did you make? (Give an overview)

Updated package.json to require:

"node": "^18.18.0 || ^20.9.0 || >=21.1.0"

This drops support for Node.js 12/14/16/17/19.

Is there anything you'd like reviewers to focus on?

Did I miss any places that should be updated?

Drops support for Node.js 12/14/16/17/19

Fixes #17595
@mdjermanovic mdjermanovic added core Relates to ESLint's core APIs and features accepted There is consensus among the team that this change meets the criteria for inclusion labels Nov 6, 2023
@eslint-github-bot eslint-github-bot bot added breaking This change is backwards-incompatible feature This change adds a new feature to ESLint labels Nov 6, 2023
Copy link

netlify bot commented Nov 6, 2023

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit 0929225
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/65511f0dc373e100083ffcd4
😎 Deploy Preview https://deploy-preview-17725--docs-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mdjermanovic mdjermanovic changed the title feat!: Require Node.js ^18.18.0 || ^20.7.0 || >=21.1.0 feat!: Require Node.js ^18.18.0 || ^20.9.0 || >=21.1.0 Nov 12, 2023
@mdjermanovic
Copy link
Member Author

This is ready for review.

@ota-meshi
Copy link
Member

What do you think about using Intl.Segmenter instead of graphemer package?

#17595 (comment)

@mdjermanovic
Copy link
Member Author

What do you think about using Intl.Segmenter instead of graphemer package?

#17595 (comment)

Makes sense to me, though I'm not sure if we should make changes to the key-spacing rule since it's now deprecated.

Would it be 100% functionally equivalent?

@ota-meshi
Copy link
Member

I think it is better to reduce the number of dependency packages even if the rules that affect them are deprecated, since it is beneficial for ESLint to reduce the number of dependency packages. What do you think?

Would it be 100% functionally equivalent?

Hmm. It seems that they are not 100% equivalent.
However, I think the purpose of both is the same. I think it would be accepted as a minor breaking change in a major version update, what do you think?

@nzakas
Copy link
Member

nzakas commented Nov 30, 2023

Let's spin off the Intl.Segmenter discussion into a separate issue so we can continue discussing.

@@ -45,7 +45,7 @@ jobs:
strategy:
matrix:
os: [ubuntu-latest]
node: [21.x, 20.x, 19.x, 18.x, 17.x, 16.x, 14.x, 12.x, "12.22.0"]
node: [21.x, 20.x, 18.x, "18.18.0"]

Choose a reason for hiding this comment

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

why not 18.x?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are both 18.x and 18.18.0.

18.x to test on the latest version of Node.js 18 (currently 18.18.2).
18.18.0 to test on the minimum supported version

@mdjermanovic
Copy link
Member Author

Let's spin off the Intl.Segmenter discussion into a separate issue so we can continue discussing.

Agreed. @ota-meshi would you like to open an issue to discuss replacing graphemer with Intl.segmenter?

@ota-meshi
Copy link
Member

Agreed. @ota-meshi would you like to open an issue to discuss replacing graphemer with Intl.segmenter?

I'll do it later when I have time.
If possible, I would like to investigate and show what kind of characters there are differences.

Copy link

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

Copy link
Member

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

I noticed several things can be updated:

sure, it's not a blocker of merging this one, we can update it in another PR later.

@mdjermanovic
Copy link
Member Author

I noticed several things can be updated:

sure, it's not a blocker of merging this one, we can update it in another PR later.

Good catches! I agree, seems best to merge this PR as-is as a starting point when we start with v9 next week, and then make individual changes like these ones and #17835 in separate PRs so that it will be easier to revisit them separately (and revert if needed; hopefully not) later.

@mdjermanovic mdjermanovic marked this pull request as ready for review December 20, 2023 12:16
@mdjermanovic mdjermanovic requested a review from a team as a code owner December 20, 2023 12:16
@mdjermanovic mdjermanovic merged commit e1e827f into main Dec 20, 2023
17 checks passed
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jun 18, 2024
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jun 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion breaking This change is backwards-incompatible core Relates to ESLint's core APIs and features feature This change adds a new feature to ESLint
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Change Request: Drop Node v12/v14/v16/v17/v19 support
8 participants