-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
chore: switched repo lint to use nx run-many #6038
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: switched repo lint to use nx run-many #6038
Conversation
Thanks for the PR, @JoshuaKGoldberg! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
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.
Pending switch over to Nx eslint executor (I'll apply that myself tomorrow)
packages/ast-spec/package.json
Outdated
@@ -34,7 +34,7 @@ | |||
"postclean": "rimraf dist && rimraf _ts3.4 && rimraf .rollup.cache && rimraf coverage", | |||
"clean-fixtures": "rimraf -g \"./src/**/fixtures/**/snapshots\"", | |||
"format": "prettier --write \"./**/*.{ts,mts,cts,tsx,js,mjs,cjs,jsx,json,md,css}\" --ignore-path ../../.prettierignore", | |||
"lint": "eslint . --ignore-path='../../.eslintignore'", | |||
"lint": "cd ../../ && nx lint @typescript-eslint/ast-spec", |
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 kept these npm script aliases as @bradzacher had previously requested back when I applied this same change to scope-manager.
Personally I run everything from the root or let the Nx Console VSCode extension give me a clickable target trigger in the project.json
of the relevant project:
(You can see it here between lines 5 and 6)
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.
Nifty, thanks. Is there a way around the cd ../..
, though? It's kind of annoying to be shoved back into the root directory. Sometimes I do want to have my shell specifically in a package dir.
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 think there are still some rough edges with running nx from a nested location, but having said that I tried the example of running:
nx lint @typescript-eslint/ast-spec
Within ./packages/ast-spec
and it worked just fine...
It's possible in simple cases like this there are no issues and/or my knowledge is out of date.
I've pushed up an additional commit which removes the cd from all of them 🤞
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.
Oh nice, thanks! I really like this 😄 very clean.
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.
One more feature request I'd have for nx
is to be able to switch this to nx lint .
in each of them. It'd be nice not to have to redundantly type the package name. Filed nrwl/nx#13289.
* chore: switched repo lint to use nx run-many * chore: replace eslint CLI usage with dedicated Nx executor * chore: remove unneeded nested .eslintignore files * chore: npm alias, try without cd to root Co-authored-by: “JamesHenry” <james@henry.sc>
PR Checklist
Overview
Uses
nx run-many
as described in #4226.I tested this locally and verified:
Note that a single uncached run is now ~34% slower in the new system. However, the much more common use case of a user only modifying the
eslint-plugin
package is ~49% faster:eslint-plugin
3Footnotes
hyperfine "cross-env NODE_OPTIONS=\"--max-old-space-size=16384\" eslint ."
↩hyperfine "nx run-many --target=lint --all --parallel --skip-nx-cache"
↩hyperfine "nx run-many --target=lint --all --parallel" --prepare "echo '/* ... */' >> packages/eslint-plugin/src/rules/no-floating-promises.ts || true"
↩hyperfine "nx run-many --target=lint --all --parallel"
↩