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

chore: update eslint from version 7 to version 8 #29355

Merged
merged 3 commits into from
Apr 26, 2024

Conversation

AtofStryker
Copy link
Contributor

@AtofStryker AtofStryker commented Apr 18, 2024

Additional details

This PR gets the monorepo on eslint v8 from v7. Because we leverage @cypress/eslint-plugin-dev quite heavily, we also needed to update @cypress/eslint-plugin-dev to support eslint version 8 and up, which is a breaking change to @cypress/eslint-plugin-dev .

Also, since eslint-plugin-json-format is no longer maintained and uses the old babel-eslint parser, I have migrated us to eslint-plugin-jsonc which is actively maintained. eslint-plugin-json-format doesn't actually have a dependency on babel-eslint, it was just a red herring of an error when trying to lint one of the system tests that is using an outdated version (intentionally) of vue-eslint-parser. I have correctly configured eslint-plugin-json-format in the repo and linted all package.json files in sub packages, which was not working correctly before.

Currently, with our release process, it is very difficult to release a standalone npm package as breaking while there are other changes inside the monorepo, which will also be flagged as breaking.

My recommendation is we merge this in as a chore, which will NOT trigger a release of @cypress/eslint-plugin-dev, and have a follow up PR that is breaking and only includes changes in the @cypress/eslint-plugin-dev directory, mainly the changelog and referencing the correct merged commit so we can easily release @cypress/eslint-plugin-dev 6.x.x

Steps to test

run yarn lint in the repo, as well as the tests inside @cypress/eslint-plugin-dev to make sure everything works correctly

Steps to review

There are quite a few changes in this PR, so I have squashed them down to two major things

  1. the updaing of eslint-plugin-dev which can be seen in the first commit. This is the bulk of important changes 309ee08
  2. the removal of eslint-plugin-json-format from the repo and the plugin and using eslint-plugin-jsonc, which changes a lot of the references in the repo. This looks like much, but most of the changes are the same. I have elected to lint the tsconfig and removed lock files from the eslint ignore since eslint-plugin-json-format does not try to lint those files by default. 2a9c95f Correctly configure eslint-plugin-json-format for use in the repo with eslint 8 and lint files that were not previously being linted correctly by the plugin 118bae2

How has the user experience changed?

mono repo users will now be on eslint v8 and opens the door to support Eslint v8 inside eslint-plugin-cypress if we still use @cypress/eslint-plugin-dev inside eslint-plugin-cypress . @MikeMcC399 is working on a PR to remove @cypress/eslint-plugin-dev (see cypress-io/eslint-plugin-cypress#177) from eslint-plugin-cypress, which is the correct direction. In the future, we might not need this plugin any longer as most the rules look to be supported OOTB from eslint-plugin-mocha (a decision to be made later).

PR Tasks

@AtofStryker
Copy link
Contributor Author

cc @MikeMcC399

Copy link

cypress bot commented Apr 18, 2024

1 flaky test on run #55197 ↗︎

0 230 4 0 Flakiness 1

Details:

properly support no duplicate imports
Project: cypress Commit: e6e262ff52
Status: Passed Duration: 11:27 💡
Started: Apr 26, 2024 6:35 PM Ended: Apr 26, 2024 6:46 PM
Flakiness  cypress/e2e/project-setup.cy.ts • 1 flaky test • launchpad-e2e

View Output

Test Artifacts
... > can skip setup CT testing for an E2E project Test Replay Screenshots

Review all test suite changes for PR #29355 ↗︎

@AtofStryker AtofStryker force-pushed the chore/update-eslint branch from 6f4a4a3 to 70c05cb Compare April 18, 2024 19:46
@MikeMcC399
Copy link
Contributor

@AtofStryker

Thanks for the cc: !

mono repo users will now be on eslint v8 and opens the door to support Eslint v8 inside eslint-plugin-cypress

Comment on lines 23 to 27
For Eslint 8+, use version 6.x.x

For Eslint 7 and below, use version 5.x.x
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about the compatibility advice!

I doubt if you have ESLint 9 compatibility set up, which requires flat files, and I don't know that getting involved with ESLint 6 and below is still worthwhile, so that would reduce to:

⚠️ Currently does not support ESLint version 9
For Eslint 8, use version 6.x.x
For Eslint 7, use version 5.x.x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would also think that is true too. I will update the readme!

@AtofStryker AtofStryker force-pushed the chore/update-eslint branch 2 times, most recently from a30fd64 to d6b086a Compare April 23, 2024 13:42
@MikeMcC399
Copy link
Contributor

@AtofStryker FYI

Copy link
Member

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

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

@AtofStryker The intention of the original json linter was to lint our package.json files to ensure they are formatted correctly and that all packages are listed alphabetically. I see that this behavior is lost with these changes.

This branch (no suggestions or fixing on save)
Screenshot 2024-04-24 at 11 25 11 AM

develop (auto fixes and alphabetizes on save)
Screenshot 2024-04-24 at 11 37 23 AM

@AtofStryker AtofStryker force-pushed the chore/update-eslint branch from 2a9c95f to 118bae2 Compare April 24, 2024 17:23
@AtofStryker
Copy link
Contributor Author

@AtofStryker The intention of the original json linter was to lint our package.json files to ensure they are formatted correctly and that all packages are listed alphabetically. I see that this behavior is lost with these changes.

This branch (no suggestions or fixing on save) Screenshot 2024-04-24 at 11 25 11 AM

develop (auto fixes and alphabetizes on save) Screenshot 2024-04-24 at 11 37 23 AM

ah I see! I only checked for syntax corrections when doing the move over. I think we can still use eslint-plugin-json-format I just needed to tune a few things in the config. It doesn't look like the sorting or linting was fulling running on the packages, so I configured it to run on all sub packages and lint the json files in 118bae2. This keeps the old plugin and reduces the scope of changes, which is a great thing!

Going to see if there are issues in CI then will update the description and request a re review

system-tests/.eslintignore Outdated Show resolved Hide resolved
@AtofStryker AtofStryker force-pushed the chore/update-eslint branch from 118bae2 to ec036c4 Compare April 24, 2024 20:11
],
rules: {
'no-duplicate-imports': 'off',
'import/no-duplicates': 'off',
'@typescript-eslint/no-duplicate-imports': [
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean duplicate imports are now allowed? I added a duplicate import to a file and yarn lint passed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

weird the rule was renamed. I'll double check and make sure I implemented it correctly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I see what happened. so the rule was deprecated for import/no-duplicates which we have set to off but we really should have it turned on. However, the rule comes from import/no-duplicates which was referenced in @cypress/eslint-plugin-dev but not installed or configured (likely resolved from the root of the monorepo). I added the dependency to the package and configured the plugin correctly, so the linting rules for duplicate imports should now work correctly. done in e6e262f

…dev minimum to eslint 7. Remove support for coffeescript and reconfigured required peer deps
…n linting on all json files (previously was not running)
@AtofStryker AtofStryker force-pushed the chore/update-eslint branch from ec036c4 to e6e262f Compare April 26, 2024 17:29
@AtofStryker AtofStryker requested a review from mschile April 26, 2024 17:31
@AtofStryker AtofStryker merged commit f14a11a into develop Apr 26, 2024
80 of 82 checks passed
@AtofStryker AtofStryker deleted the chore/update-eslint branch April 26, 2024 18:42
jj497 pushed a commit to jj497/cypress that referenced this pull request May 5, 2024
* chore: (for eslint-plugin-dev only is breaking) update eslint-plugin dev minimum to eslint 7. Remove support for coffeescript and reconfigured required peer deps

* correctly configure eslint-plugin-json-format for the monorepo and run linting on all json files (previously was not running)

* properly support no duplicate imports
@cypress-bot
Copy link
Contributor

cypress-bot bot commented May 8, 2024

Released in 13.9.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v13.9.0, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators May 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Cypress repo to Eslint version 8 and have @cypress/eslint-plugin-dev support eslint v8
4 participants