Skip to content

ESLint v8 (#35576) - #36283

Merged
gziolo merged 19 commits into
WordPress:trunkfrom
simonhammes:eslint-v8
Dec 3, 2021
Merged

ESLint v8 (#35576)#36283
gziolo merged 19 commits into
WordPress:trunkfrom
simonhammes:eslint-v8

Conversation

@simonhammes

@simonhammes simonhammes commented Nov 6, 2021

Copy link
Copy Markdown
Contributor

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).

Comment thread packages/api-fetch/src/test/index.js Outdated
Comment thread packages/block-serialization-default-parser/test/index.js Outdated
Comment thread packages/env/lib/config/test/config.js Outdated
Comment thread packages/eslint-plugin/rules/data-no-store-string-literals.js Outdated
@simonhammes simonhammes changed the title WIP: ESLint v8 WIP: ESLint v8 (#35576) Nov 6, 2021
@gziolo gziolo added [Tool] 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 commented Nov 8, 2021

Copy link
Copy Markdown
Member

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.

@gziolo gziolo left a comment

Copy link
Copy Markdown
Member

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.

Comment thread packages/eslint-plugin/configs/recommended.js Outdated
@simonhammes

Copy link
Copy Markdown
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

gziolo commented Nov 11, 2021

Copy link
Copy Markdown
Member

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

@simonhammes

Copy link
Copy Markdown
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

gziolo commented Nov 19, 2021

Copy link
Copy Markdown
Member

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
Copy Markdown
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

gziolo commented Dec 1, 2021

Copy link
Copy Markdown
Member

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.

Comment thread package-lock.json Outdated
Comment thread packages/eslint-plugin/package.json Outdated
Comment thread test/unit/jest.config.js Outdated
@gziolo

gziolo commented Dec 2, 2021

Copy link
Copy Markdown
Member

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>

@gziolo gziolo left a comment

Copy link
Copy Markdown
Member

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
@github-actions github-actions Bot added this to the Gutenberg 12.2 milestone Dec 3, 2021
@gziolo

gziolo commented Dec 22, 2021

Copy link
Copy Markdown
Member

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

[Tool] ESLint plugin /packages/eslint-plugin [Type] Breaking Change For PRs that introduce a change that will break existing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

2 participants