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
@mdjermanovic mdjermanovic deleted the issue17595 branch December 20, 2023 13:02
bmish added a commit to bmish/eslint that referenced this pull request Dec 27, 2023
* main: (25 commits)
  test: ensure that CLI tests run with FlatESLint (eslint#17884)
  fix!: Behavior of CLI when no arguments are passed (eslint#17644)
  docs: Update README
  Revert "feat!: Remove CodePath#currentSegments" (eslint#17890)
  feat!: Update shouldUseFlatConfig and CLI so flat config is default (eslint#17748)
  feat!: Remove CodePath#currentSegments (eslint#17756)
  chore: update dependency markdownlint-cli to ^0.38.0 (eslint#17865)
  feat!: deprecate no-new-symbol, recommend no-new-native-nonconstructor (eslint#17710)
  feat!: check for parsing errors in suggestion fixes (eslint#16639)
  feat!: assert suggestion messages are unique in rule testers (eslint#17532)
  feat!: `no-invalid-regexp` make allowConstructorFlags case-sensitive (eslint#17533)
  fix!: no-sequences rule schema correction (eslint#17878)
  feat!: Update `eslint:recommended` configuration (eslint#17716)
  feat!: drop support for string configurations in flat config array (eslint#17717)
  feat!: Remove `SourceCode#getComments()` (eslint#17715)
  feat!: Remove deprecated context methods (eslint#17698)
  feat!: Swap FlatESLint-ESLint, FlatRuleTester-RuleTester in API (eslint#17823)
  feat!: remove formatters except html, json(-with-metadata), and stylish (eslint#17531)
  feat!: Require Node.js `^18.18.0 || ^20.9.0 || >=21.1.0` (eslint#17725)
  fix: allow circular references in config (eslint#17752)
  ...
@@ -170,6 +170,6 @@
],
"license": "MIT",
"engines": {
"node": "^12.22.0 || ^14.17.0 || >=16.0.0"
"node": "^18.18.0 || ^20.9.0 || >=21.1.0"

Choose a reason for hiding this comment

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

Hey sorry for the random comment but why ^18.18.0 and not ^18.0.0? I've been trying to dig around for this and can't find the reason. I've got the node engine set to >=18 in my app so I'd need to upgrade which is fine, I'm just curious :)

Choose a reason for hiding this comment

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

It's the first LTS release for 18

Copy link

@koddsson koddsson Jan 10, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

have you gone through the discussion in the related issue?

Choose a reason for hiding this comment

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

have you gone through the discussion in the related issue?

Yeah but I'm missing why those versions were chosen. The only thing I see is it being the latest versions at the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion 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
Status: Complete
Development

Successfully merging this pull request may close these issues.

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