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

Breaking: drop node v10/v13/v15 (fixes #14023) #14592

Merged
merged 8 commits into from Aug 5, 2021

Conversation

aladdin-add
Copy link
Member

@aladdin-add aladdin-add commented May 16, 2021

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 autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:

What changes did you make? (Give an overview)

fixes #14023

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

no.

@aladdin-add aladdin-add 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 breaking This change is backwards-incompatible labels May 16, 2021
@aladdin-add aladdin-add reopened this May 16, 2021
.github/workflows/ci.yml Outdated Show resolved Hide resolved
lib/shared/relative-module-resolver.js Outdated Show resolved Hide resolved
Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

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

LGTM whenever we start merging breaking changes for v8.0.0.

package.json Outdated Show resolved Hide resolved
@mdjermanovic mdjermanovic changed the title Breaking: drop node v10/v13 (fixes #14023) Breaking: drop node v10/v13/v15 (fixes #14023) Jun 8, 2021
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

We should also update prerequisites in README and getting-started.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mdjermanovic mdjermanovic mentioned this pull request Jul 5, 2021
1 task
@btmills
Copy link
Member

btmills commented Jul 6, 2021

Note to whomever merges this: try reverting #14771, which pinned fs-teardown to a version that still supports Node 10.12.

@aladdin-add
Copy link
Member Author

#14771 has been merged. so I can revert it after rebasing.

@mdjermanovic
Copy link
Member

Re fs-teardown, when we drop Node 10 it may be even better to set "fs-teardown": "^0.1.3" instead of reverting to ^0.1.0. That will leave v0.1.2 (which trims file contents) out of the range.

@aladdin-add
Copy link
Member Author

rebased upstream master.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

verify_files doesn't specify Node version, and the current default seems to be v10.24.1. After merging this PR, that job will be running on an unsupported version.

https://github.com/eslint/eslint/pull/14592/checks?check_run_id=3067449292 was successful with warnings, but the check will eventually break when we add more code that doesn't work in Node 10.

npm WARN notsup Unsupported engine for eslint@7.30.0: wanted: {"node":"^12.22.0 || ^14.17.0 || >=16.0.0"} (current: {"node":"10.24.1","npm":"6.14.12"})
npm WARN notsup Not compatible with your version of node/npm: eslint@7.30.0
npm WARN notsup Unsupported engine for fs-extra@10.0.0: wanted: {"node":">=12"} (current: {"node":"10.24.1","npm":"6.14.12"})
npm WARN notsup Not compatible with your version of node/npm: fs-extra@10.0.0

I think we should specify the Node version, but not sure which one would make the most sense - 12.22.0, 12.x, 14.x, or 16.x?

@nzakas
Copy link
Member

nzakas commented Jul 21, 2021

Seems like 14.x would be the safest bet? Guaranteed ESM and stable.

@ErisDS
Copy link

ErisDS commented Oct 21, 2021

Just wondering if there was a particular reason for pinning to 4.17.0 rather than an earlier version of 14?

Our project is an installed application, rather than a library & is currently pinned to 14.16.1. For us bumping Node to be able to upgrade a dev dependency doesn't make sense as it would impact 1000s of people 😬

Is there any chance you'd accept a PR to drop the pinned version by 1 version to 14.16.1?

ErisDS added a commit to ErisDS/eslint that referenced this pull request Oct 21, 2021
refs eslint#14592 (comment)

- this will allow the Ghost project to update their version of EsLint without impacting on all of our users
@aladdin-add
Copy link
Member Author

it was discussed in #14023 (comment).

a workaround is to use --ignore-engines, it may work well, but we cannot guarantee it will be working in all future versions.

@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Feb 2, 2022
@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 Feb 2, 2022
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

breaking: drop node v10/v13/v15 support
7 participants