-
Notifications
You must be signed in to change notification settings - Fork 3.3k
chore: Eslint migration foundation + migrate npm/grep package #32046
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
Conversation
5c327f5
to
831af8d
Compare
831af8d
to
8c27e00
Compare
@MikeMcC399 Might be curious to get your eyes on this (if you have time), since you've been handling eslint config in the other repos so nicely. Particularly the guide outline. |
cypress
|
Project |
cypress
|
Branch Review |
eslint-migration-foundation
|
Run status |
|
Run duration | 19m 23s |
Commit |
|
Committer | Cacie Prins |
View all properties for this run ↗︎ |
Test results | |
---|---|
|
0
|
|
11
|
|
1232
|
|
0
|
|
32177
|
View all changes introduced in this branch ↗︎ |
UI Coverage
45.96%
|
|
---|---|
|
189
|
|
165
|
Accessibility
97.95%
|
|
---|---|
|
3 critical
8 serious
2 moderate
2 minor
|
|
107
|
packages/eslint-config/package.json
Outdated
"eslint-import-resolver-typescript": "3.8.2", | ||
"eslint-plugin-cypress": "^4.1.0", | ||
"eslint-plugin-import-x": "^4.6.1", | ||
"eslint-plugin-mocha": "^10.5.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider upgrading to eslint-plugin-mocha@11
I only have some general comments:
|
@MikeMcC399 Thank you so much for the feedback! |
@MikeMcC399 I'm considering whether, after the migration in this repo, switching eslint-config out to the npm package as a major version upgrade. It's not right now to allow for a progressive migration in this repo, though. Thanks for bringing that up! |
}, | ||
"include": ["e2e/**/*.ts"] | ||
"include": [ | ||
"**/*.ts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you remember why we needed to touch the tsconfig here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To get eslint to pick up on everything. The new eslint config runs .js files through typescript linter - this helps it lint interactions between js and ts. If typescript eslint matches a file that isn't in the "default project" (e.g., not matched by the closest tsconfig), it throws an error.
"lint": "eslint" | ||
}, | ||
"dependencies": { | ||
"jiti": "^2.4.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is jiti a peer dependency or something that we need to include here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jiti is a dependency of an updated eslint plugin, but a few dependencies of frontend-shared
depend on an older version of jiti. Yarn doesn't install the version that one of the eslint pugins here needs, so node uses thet older version of jiti via hoisting. Declaring it here prevents it from using the hoisted version.
|
||
{ | ||
// rules that are gold standard, but have many violations | ||
// these are off while developing eslint, but should eventually be enabled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in other words we need to disable these rules temporarily because there is an overwhelming amount of errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, they're not currently rules that are applied to the project, but ones that would be good to turn on - they are defaulted on in the recommended rules.
… from eslint-config
…on - once migration is complete, this will clean up
Additional details
This is a pared down PR related to #30892
It includes:
npm/grep
./npm/grep
Steps to test
How has the user experience changed?
PR Tasks
cypress-documentation
?type definitions
?