-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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: improve linting #25235
chore: improve linting #25235
Conversation
Thanks for taking the time to open a PR!
|
Test summaryRun details
View run in Cypress Dashboard ➡️ Flakiness
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
@@ -1,6 +1,7 @@ | |||
/// <reference types="cypress" /> | |||
|
|||
declare namespace Cypress { | |||
// eslint-disable-next-line @typescript-eslint/no-unused-vars |
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.
I'm not totally sure what the right thing to do here is, I made this change to fix the lint error from no-unused-vars
but this doesn't look exactly right. Should E
on line 14 extend Subject
instead of HTMLElement
? @jordanpowell88 any ideas?
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.
Doe we need Chainable
to have Subject
? We might be able to omit it. Should it just be
declare namespace Cypress {
interface Chainable {
/**
* Get one or more DOM elements by an XPath selector.
* **Note:** you can test XPath expressions from DevTools console using $x(...) function, for example $x('//div') to find all divs.
* @see https://github.com/cypress-io/cypress-xpath
* @example
* cy.xpath(`//ul[@class="todo-list"]//li`)
* .should('have.length', 3)
*/
xpath<E extends Node = HTMLElement>(selector: string, options?: Partial<Loggable & Timeoutable>): Chainable<JQuery<E>>
}
}
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.
We have some examples in the docs that don't pass Subject
to Chainable
: https://docs.cypress.io/guides/tooling/typescript-support#Types-for-Custom-Commands
"js-codemod": "cpojer/js-codemod", | ||
"jscodemods": "https://github.com/cypress-io/jscodemods.git#01b546e", | ||
"jscodeshift": "0.7.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.
Removing these 3 historical deps got rid of 249 transitive dependencies and 74 MB:
Before:
~ echo **/node_modules/* | wc -w
7371
~ du -c -B M **/node_modules/*
...
2792M total
After:
~ echo **/node_modules/* | wc -w
7122
~ du -c -B M **/node_modules/*
...
2718M total
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.
Nice!
I wonder if there's a tool we can use to detect unused dependencies and prune more?
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.
Yeah, it's called depcheck: https://github.com/depcheck/depcheck I want to add this to the repo at some point for sure.
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.
Code seems fine. I tried doing yarn lint-changed
but I got a strange error:
$ yarn lint-changed simplify-linting
yarn run v1.22.11
$ lint-changed
Oops! Something went wrong! :(
ESLint: 7.22.0
Error: 'patterns' must be a non-empty string or an array of non-empty strings
at ESLint.lintFiles (/home/lachlan/code/work/dev/node_modules/eslint/lib/eslint/eslint.js:524:19)
at Object.execute (/home/lachlan/code/work/dev/node_modules/eslint/lib/cli.js:296:36)
at main (/home/lachlan/code/work/dev/node_modules/eslint/bin/eslint.js:142:52)
at Object.<anonymous> (/home/lachlan/code/work/dev/node_modules/eslint/bin/eslint.js:146:2)
at Module._compile (node:internal/modules/cjs/loader:1155:14)
at Object.Module._extensions..js (node:internal/modules/cjs/loader:1209:10)
at Module.load (node:internal/modules/cjs/loader:1033:32)
at Function.Module._load (node:internal/modules/cjs/loader:868:12)
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
at node:internal/main/run_main_module:22:47
error Command failed with exit code 1.
I'm not sure if this ever worked, though, or if anyone uses it. The pre-commit lint hook still works fine, I tested it out.
"js-codemod": "cpojer/js-codemod", | ||
"jscodemods": "https://github.com/cypress-io/jscodemods.git#01b546e", | ||
"jscodeshift": "0.7.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.
Nice!
I wonder if there's a tool we can use to detect unused dependencies and prune more?
This reverts commit d4fd6df.
@lmiller1990 It seems broken in |
User facing changelog
Additional details
.eslintrc
but noyarn lint
command, meaning linting only happened in the IDE, not in CI. Fixed this where needed.Steps to test
How has the user experience changed?
PR Tasks
cypress-documentation
?type definitions
?