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

ESLint v8 (#35576) #36283

Merged
merged 19 commits into from Dec 3, 2021
Merged

ESLint v8 (#35576) #36283

merged 19 commits into from Dec 3, 2021

Conversation

simonhammes
Copy link
Contributor

@simonhammes simonhammes commented Nov 6, 2021

Description

Closes #35576.

@wordpress/eslint-plugin

  • Updated packages for compatibility with ESLint v8

@wordpress/scripts

How has this been tested?

I only just noticed that @gziolo was working on this, so some changes in this PR overlap with changes in #36244.

I will also add changelog entries before this gets merged.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@simonhammes simonhammes changed the title WIP: ESLint v8 WIP: ESLint v8 (#35576) Nov 6, 2021
@gziolo gziolo added [Package] ESLint plugin /packages/eslint-plugin [Type] Breaking Change For PRs that introduce a change that will break existing functionality labels Nov 6, 2021
@gziolo gziolo added this to Needs decision in Core JS Nov 6, 2021
@gziolo gziolo moved this from Needs decision to To do in Core JS Nov 6, 2021
@gziolo gziolo moved this from To do to In progress in Core JS Nov 6, 2021
@gziolo
Copy link
Member

gziolo commented Nov 8, 2021

Thank you for starting this PR.

I only just noticed that @gziolo was working on this, so some changes in this PR overlap with changes in #36244.

I expect this PR will land this week. I will let you know when it happens so you could rebase this one.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

This is looking good. We still need to document all the major version updates for packages as breaking changes. We have some fixes included in this PR which confirm that upgrade for the new version of the @wordpress/eslint-plugin won't be seamless.

Let's see which errors will still require some action after merging changes for Babel v7.16.0.

packages/eslint-plugin/configs/recommended.js Show resolved Hide resolved
@simonhammes
Copy link
Contributor Author

This is looking good. We still need to document all the major version updates for packages as breaking changes.

I will document those changes 👍

@gziolo
Copy link
Member

gziolo commented Nov 11, 2021

I landed #36244. Let’s see if rebasing is going to be enough to make all Continues Integration checks pass.

@simonhammes
Copy link
Contributor Author

I landed #36244. Let’s see if rebasing is going to be enough to make all Continues Integration checks pass.

I rebased this PR.
Unfortunately, all eslint-plugin tests are still failing: https://github.com/WordPress/gutenberg/runs/4180615328?check_suite_focus=true#step:5:908

I tried to debug the issue and they might be related to the following issues/PRs:

It seems like the testing failures arise from an interaction between Jest & the upgrade from ESLint from v7 to v8.

I suspect that the following PR made to ESLint is the main culprit: eslint/eslint#14706 (since Jest does not seem to support the exports field in package.json: jestjs/jest#9771).

I could get the tests for the eslint-plugin to pass locally by adding an /** @jest-environment node */ annotation (to force Jest to use the node environment instead of the default jsdom environment) to the top of each test file in the packages/eslint-plugin/rules/__tests__ directory and running the tests directly with Jest (command: node node_modules/.bin/jest packages/eslint-plugin/). But this is not a real solution.

I also tried to upgrade ESLint to v8 on this branch: #33287 to eliminate the possibility of a possible incompatibility between ESLint v8 and Jest v26, but the tests also failed with the same error message as in the linked CI run at the top of this comment.

@gziolo
Copy link
Member

gziolo commented Nov 19, 2021

It looks like there are some incompatible changes introduced between ESLint v8 and Jest v27 that will force us to update both tools at a similar time. I also encountered several issues when trying to upgrade Jest to v27, but I managed to resolve most of them. The only blocker is related to React Native so we need to wait until the Mobile team brings React Native to the more recent version. Do you think we will have to wait until Jest v27 is there before we can upgrade ESLint to v8?

@simonhammes
Copy link
Contributor Author

@gziolo I managed to solve the issue with the failing unit tests.

However, the "Static Analysis" job is failing because of an uncommitted change to package-lock.json.

The job was failing earlier, then I ran npm install locally and committed the change: 920598e

Now the job is failing with the exact same opposite diff.

@simonhammes simonhammes marked this pull request as ready for review December 1, 2021 21:06
@simonhammes simonhammes changed the title WIP: ESLint v8 (#35576) ESLint v8 (#35576) Dec 1, 2021
@gziolo
Copy link
Member

gziolo commented Dec 1, 2021

I managed to solve the issue with the failing unit tests.

Great news! 🎉 Awesome work 👏

The job was failing earlier, then I ran npm install locally and committed the change: 920598e

Yes, I’m seeing it a lot recently. We really need to upgrade to npm 8 once the next major WordPress release (5.9 is scheduled for January) is out.

I think you can manually revert the changes in the lock file that get reported in the CI job.

@gziolo gziolo moved this from In progress to Needs review in Core JS Dec 1, 2021
package-lock.json Outdated Show resolved Hide resolved
test/unit/jest.config.js Outdated Show resolved Hide resolved
@gziolo
Copy link
Member

gziolo commented Dec 2, 2021

I guess we can land this PR pretty soon once the remaing issues/questions get addressed.

simonhammes and others added 19 commits December 2, 2021 21:00
Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Great work with the ESLint upgrade. Everything looks good, and it seems to work correctly. Let’s land it as soon as Continuous Integration turns green.

@gziolo gziolo merged commit abec4ed into WordPress:trunk Dec 3, 2021
@gziolo gziolo moved this from Needs review to Done in Core JS Dec 3, 2021
@github-actions github-actions bot added this to the Gutenberg 12.2 milestone Dec 3, 2021
@gziolo
Copy link
Member

gziolo commented Dec 22, 2021

It's now available for testing on npm with:

npm install --save-dev @wordpress/eslint-plugin@next

or

npm install --save-dev @wordpress/scripts@next

The stable version should be available in the second half of January.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] ESLint plugin /packages/eslint-plugin [Type] Breaking Change For PRs that introduce a change that will break existing functionality
Projects
No open projects
Core JS
  
Done
Development

Successfully merging this pull request may close these issues.

[@wordpress/eslint-plugin] Support for ESlint v8
2 participants