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

refactor: Unify eslint config, convert to flat configs and update plugins (WIP) #30892

Open
wants to merge 115 commits into
base: develop
Choose a base branch
from

Conversation

cacieprins
Copy link
Contributor

@cacieprins cacieprins commented Jan 16, 2025

Additional details

This is a first step to working towards our Typescript north star.

The big, structural changes:

  • npm/eslint-plugin-dev is removed. The custom rules defined there are available from eslint-plugin-cypress and eslint-plugin-mocha, or were ignored in places they would have applied.
  • Eslint is updated to 9.18
  • Eslint config is converted to the newer flat format
  • eslint-stylistic is used for formatting rules that were deprecated. We should consider moving the prettier, but this is a good first step.
  • Recommended rulesets are imported from the plugins we leverage
    • "Gold standard" rules that fail against the repo are turned off, and should be turned to 'warn' before merging. They're in a special section in the baseConfig.
    • Stylistic rules that were previously applied inconsistently across the repo are turned off, to limit the diff size of this PR. They should be enabled in a follow-up PR.

Benefits:

  • Working towards enabling gold standard rules will be easier
  • Using a base 'recommended' ruleset from the plugins we use aligns standards with other projects
  • Fewer complicated special-cases for rules
  • Dogfooding our own eslint-plugin-cypress

Some drawbacks:

  • GraphQL linting is not as comprehensive. This is mostly due to the major version of graphql that the repo and some of its other related dependencies require. Upgrading graphql is out of scope for the forseeable future - it will require an enormous reworking of packages/data-context and packages/graphql. Because this area of the code isn't currently modified very often, I think this is an okay tradeoff.
  • Vue linting is slow. That's because it's being processed both by ts and by vue. It takes approximately 1 second per .vue file. This will increase CI times for linting in several of our packages. See: Very slow with typescript-eslint parser and project vuejs/vue-eslint-parser#65

Todo:

  • Ensure autofix did not break any builds. There were changes to tsconfigs, which may impact this:
    • typescript-eslint parses .js files too, so that enabling recommended typings goes smoother - ts files that import js files can get some modicum of checking.
    • Several packages have new tsconfig.json files in their cypress/ and/or test/ directories. This is to help typescript-eslint's projectServer functionality determine which files should be included or excluded from the project (and thus linting)
  • Several packages' eslint.config.ts files may have redundancies with the baseConfig in the root eslint.config.ts file. These are from partial application of DRY's "rule of three," and can probably be removed.
  • Turn "gold standard" rules to "warn" - they were turned off to more easily resolve errors. Those that can be --fixed will likely result in very large diffs, so switching them on may warrant additional PRs.

Steps to test

How has the user experience changed?

PR Tasks

{
// rules that are gold standard, but have many violations
// these are off while developing eslint, but will be set to warn
rules: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These should be ordered by:

  1. originating ruleset (cypress/ before react/, etc)
  2. impact of not fixing the rule.: rules that help prevent runtime errors or bugs should be prioritized over more superficial rules.

@MikeMcC399
Copy link
Contributor

@cacieprins

@cacieprins cacieprins changed the title refactor: Unify eslint config, convert to flat configs and update plugins refactor: Unify eslint config, convert to flat configs and update plugins (WIP) Jan 16, 2025
"prettier.requireConfig": true,
"prettier.disableLanguages": [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these options are no longer valid in vscode

@cacieprins cacieprins marked this pull request as ready for review January 21, 2025 16:57
@@ -1,8 +1,17 @@
{
"private": true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file was re-sorted by prettier-package-json https://www.npmjs.com/package/prettier-package-json

package.json Outdated Show resolved Hide resolved
@cacieprins cacieprins requested review from AtofStryker and ryanthemanuel and removed request for AtofStryker January 21, 2025 17:00
Copy link

cypress bot commented Jan 21, 2025

cypress    Run #59949

Run Properties:  status check failed Failed #59949  •  git commit 9f30b8543d: ignore validations in system tests
Project cypress
Branch Review eslint-stylistic
Run status status check failed Failed #59949
Run duration 12m 33s
Commit git commit 9f30b8543d: ignore validations in system tests
Committer Cacie Prins
View all properties for this run ↗︎

Test results
Tests that failed  Failures 134
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 155
Tests that did not run due to a failure in a mocha hook  Skipped 54
Tests that passed  Passing 4233
View all changes introduced in this branch ↗︎

Warning

Partial Report: The results for the Application Quality reports may be incomplete.

UI Coverage  44.72%
  Untested elements 89  
  Tested elements 72  
Accessibility  96.17%
  Failed rules  0 critical   4 serious   0 moderate   0 minor
  Failed elements 199  

Tests for review

Failed  commands/net_stubbing.cy.ts • 0 failed tests • 5x-driver-firefox

View Output

Test Artifacts
Failed  commands/actions/type_special_chars.cy.js • 0 failed tests • 5x-driver-firefox

View Output

Test Artifacts
Failed  commands/cookies.cy.js • 0 failed tests • 5x-driver-firefox

View Output

Test Artifacts
Failed  e2e/origin/cookie_behavior.cy.ts • 0 failed tests • 5x-driver-firefox

View Output

Test Artifacts
Failed  commands/querying/querying.cy.js • 0 failed tests • 5x-driver-firefox

View Output

Test Artifacts

The first 5 failed specs are shown, see all 1182 specs in Cypress Cloud.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove dependency on ESLint v8 (EOL)
2 participants