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

fix(linter): disabled eslint rules dont count as type checked rules #6897

Merged

Conversation

kylecannon
Copy link
Contributor

@kylecannon kylecannon commented Aug 30, 2021

Current Behavior

#5798 migration sped up eslint for projects that don't need type checking. After spending several hours debugging, I found that the migration was not properly being ran for my workspace. It turned out if I remove the parserOptions.project in my .eslintrc.json file, I would receive no linting errors and reduced the run time from over 3 minutes to under 15 seconds. Upon digging into my .eslintrc.json file and looking at how the migration was done, oversight for overrides that turn off rules weren't accounted for and continued to keep the full type checking mode in the workspace project.

Expected Behavior

The migration should not consider override rules that set the rule to off to be an enabled rule.

To solve the issue on future NX releases, I created a migration that re-runs the previous 12.4.0 migration again with the fixes to save duplication of code and included an additional unit test for the edge case. If this is not acceptable please let me know the best way to go about having the next NX release re-run the previous migration for the next 12.9 release.

@vercel
Copy link

vercel bot commented Aug 30, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/nrwl/nx-dev/13DfLewYp51kNqeCN7NQotmToQsA
✅ Preview: Canceled

[Deployment for 166c03d canceled]

@kylecannon
Copy link
Contributor Author

I am not sure how https://nx.app/runs/SgMVzRnfgbe is failing as I didn't touch anything related to styled components.

@JamesHenry
Copy link
Collaborator

@kylecannon I also got hit by the nextjs failure, I believe I tracked it down to this: #6900

@kylecannon
Copy link
Contributor Author

@JamesHenry good to know. I will rebase once it's merged.

@JamesHenry
Copy link
Collaborator

We actually think #6903 is more likely the fix for CI now

@kylecannon kylecannon force-pushed the fix/eslint-typechecking-rules-migration branch from 1819aa9 to 166c03d Compare August 31, 2021 18:42
@kylecannon
Copy link
Contributor Author

Thanks for the help @JamesHenry. My rebase passes 🥳

Copy link
Collaborator

@FrozenPandaz FrozenPandaz left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! LGTM 🎉

@FrozenPandaz FrozenPandaz merged commit 40532a2 into nrwl:master Aug 31, 2021
@maxisam
Copy link

maxisam commented Sep 8, 2021

It doesn't work. I just create a new nx project without parserOptions in eslint. And it will throw errors like

Parsing error: Cannot read file 'test\nx1290\tsconfig.json'.

I think the migration shouldn't remove all parserOptions. It causes issue like this.

When you create a new project

npx create-nx-workspace --preset=angular and remove parserOptions in eslint, you will see the same error.

@kylecannon kylecannon deleted the fix/eslint-typechecking-rules-migration branch September 8, 2021 15:49
@flippidippi
Copy link

We are extending standard-with-typescript which requires parserOptions and this migration removed all of them incorrectly ☹️

@kylecannon
Copy link
Contributor Author

@JamesHenry im a little stumped what the root cause is ^ I basically re ran the old migration but only checked for 'off'

@github-actions
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 20, 2023
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.

None yet

5 participants