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

Jump to bottom

Use the built-in class fields and private methods rules in ESLint 8 #14872

Merged
merged 14 commits into from Sep 13, 2022
Merged

Use the built-in class fields and private methods rules in ESLint 8 #14872

merged 14 commits into from Sep 13, 2022

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Aug 23, 2022 •
edited

Q                       A
Fixed Issues? Fixes #14496
Patch: Bug Fix? ESLint rule @babel/semi conflicts with ESLint 8.0.0 when reporting class properties trailing commas
Major: Breaking Change? Yes
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes? eslint-scope is bumped to 7.1.1 and eslint-visitor-keys to 3.3.0
License MIT

Bumped eslint-scope and eslint-visitor-keys. Because they drop Node.js 10 support, the update is behind the BABEL_8_BREAKING flag.

This PR also bumps eslint dev dependencies to ESLint 8 and fixes compatibility issues of the following rules:

  • semi
  • no-invalid-this

Because ESLint 8 officially supported class properties / private methods. These rules become no-op when using with ESLint 8.

In this PR we also introduce a new test workflow eslint7-test where we downgrade eslint to 7 and run the eslint related tests. The test is refactored so that we can test eslint parser/plugins against either ESLint 7 or ESLint 8, but not both. Compared to the current approach, the new approach

  1. avoid the test interference when one test worker is testing eslint plugins with ESLint 8 and another worker is testing @babel/eslint-parser with ESLINT_VERSION_FOR_BABEL = 7
  2. test other @babel/eslint-* with both ESLint 7 and ESLint 8
  3. simplify the build process by removing a customized Babel plugin

Hopefully we can remove this workflow when we decide to drop ESLint 7 support in the future.

I wish we could match the Node.js release frequency, otherwise when our deps drop unmaintained Node.js versions support, we have to either 1) pin older versions because we can't drop EOL node versions in minors, or 2) fork the legacy dep and offer runtime switch like we did for chokidar@2.

@babel-bot
Copy link
Collaborator

babel-bot commented Aug 23, 2022 •
edited

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/52920/

existentialism
existentialism previously approved these changes Aug 23, 2022
View reviewed changes
@JLHwung
Copy link
Contributor Author

JLHwung commented Aug 23, 2022

The CI error is related: It seems like a package manager issue, I am investigating.

@JLHwung
Copy link
Contributor Author

JLHwung commented Aug 24, 2022

OK. CI is failing because the sub module import eslint/lib/definitions is not proxied in the yarn-plugin-conditions wrapper package. @nicolo-ribaudo I think supporting undeclared sub imports will be an overkill for that plugin, should we fork eslint-scope 5.1?

@nicolo-ribaudo
Copy link
Member

@JLHwung Instead of forking I just created a small proxy package: https://github.com/nicolo-ribaudo/eslint-scope-5-internals

@nicolo-ribaudo nicolo-ribaudo changed the title Bump eslint parser deps [babel 8] Bump eslint parser deps Sep 5, 2022
@JLHwung
Copy link
Contributor Author

JLHwung commented Sep 6, 2022

The current E2E test is failing because ESLint v8 supports class properties / private methods, but babel-eslint-plugin is not aware of that and thus reports essentially duplicate errors.

We can detect ESLint v8 and make the following rules as no-op:

semi
no-invalid-this
object-curly-spacing

I don't think we should remove rules, in case in the future new syntaxes require extension again.

@JLHwung JLHwung marked this pull request as draft September 7, 2022 19:02
@JLHwung JLHwung force-pushed the bump-eslint-parser-deps branch 2 times, most recently from 295dfa3 to 0df1b61 Compare September 9, 2022 19:00
@JLHwung JLHwung marked this pull request as ready for review September 9, 2022 19:31
@JLHwung JLHwung added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Sep 9, 2022
JLHwung
JLHwung commented Sep 9, 2022
View reviewed changes
eslint/babel-eslint-parser/test/index.js Outdated
if (eslintVersion !== 8) {
if (isESLint7) {
if (process.env.IS_PUBLISH) {
console.warn("Skipping ESLint 7 test because using a release build.");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now the published build will test against ESLint 8 only.

JLHwung
JLHwung commented Sep 9, 2022
View reviewed changes
eslint/babel-eslint-parser/test/index.js
...espreeOptions,
ecmaVersion: 2022,
ecmaVersion: "latest",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ESLint 8 also supports ES2023 so I put "latest" here.

nicolo-ribaudo
nicolo-ribaudo approved these changes Sep 13, 2022
View reviewed changes
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Thank you!

@JLHwung
Copy link
Contributor Author

JLHwung commented Sep 13, 2022

Rebased on latest main.

@nicolo-ribaudo nicolo-ribaudo merged commit 091ac3d into babel:main Sep 13, 2022
@nicolo-ribaudo nicolo-ribaudo deleted the bump-eslint-parser-deps branch September 13, 2022 16:18
@nicolo-ribaudo nicolo-ribaudo removed the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Sep 13, 2022
@nicolo-ribaudo
Copy link
Member

I removed the label so that it doesn't go in the changelog (because this pr is for Babel 8)

@JLHwung
Copy link
Contributor Author

JLHwung commented Sep 13, 2022

Well the bug fix commit 37ef28c will be shipped to Babel 7 anyway.

@nicolo-ribaudo nicolo-ribaudo added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Sep 14, 2022
@nicolo-ribaudo nicolo-ribaudo changed the title [babel 8] Bump eslint parser deps Use the built-in class fields and private methods rules in ESLint 8 Sep 14, 2022
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Dec 14, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 14, 2022
@JLHwung JLHwung added the PR: Internal (next major) 🏠 A type of pull request used for our changelog categories for next major release label Aug 9, 2023
@JLHwung JLHwung added this to the v8.0.0 milestone Aug 9, 2023
@JLHwung JLHwung removed the babel 8 label Aug 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo approved these changes

@liuxingbaoyu liuxingbaoyu liuxingbaoyu approved these changes

@existentialism existentialism existentialism left review comments

Assignees
No one assigned
Labels
area: eslint outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories PR: Dependency ⬆️ PR: Internal (next major) 🏠 A type of pull request used for our changelog categories for next major release
Projects
None yet
Milestone
v8.0.0
Development

Successfully merging this pull request may close these issues.

Update @babel/eslint-parser use of eslint-scope to the latest version
5 participants
@JLHwung @babel-bot @nicolo-ribaudo @existentialism @liuxingbaoyu