-
Notifications
You must be signed in to change notification settings - Fork 26.1k
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
feat: enable @typescript-eslint/recommended-type-checked in create-next-app --typescript #52845
base: canary
Are you sure you want to change the base?
feat: enable @typescript-eslint/recommended-type-checked in create-next-app --typescript #52845
Conversation
d04e5aa
to
6c62e53
Compare
docs/02-app/01-building-your-application/06-configuring/02-eslint.mdx
Outdated
Show resolved
Hide resolved
docs/02-app/01-building-your-application/06-configuring/02-eslint.mdx
Outdated
Show resolved
Hide resolved
…nternally (#52948) Spinning out from #37151 and my draft PR #52845, this enables the two basic recommended rulesets from [typescript-eslint](https://typescript-eslint.io) for the Next.js monorepo source code: * [`plugin:@typescript-eslint/recommended`](https://typescript-eslint.io/linting/configs#recommended): Our base recommended rules that detect common bugs or _(non-stylistic)_ TypeScript bad practices * [`plugin:@typescript-eslint/stylistic`](https://typescript-eslint.io/linting/configs#stylistic): Our base starting stylistic recommended for keeping codebases visually consistent and avoiding out-of-practice visual constructs The process I used is pretty standard (see typescript-eslint/typescript-eslint#6760 for other repos it was done on): 1. Enable those base recommended presets 2. Remove any rule settings that are now redundant 3. Reconfigure any rule whose default settings didn't seem to make sense for this codebase 4. Add a `// Todo: ...` comment, and under it, add a disable for any rule that immediately reported a lot of complaints Note that this only enables the presets internally. It doesn't impact what end-users of published packages such as Next.js or `create-next-app` experience. That's a separate task in #52845. I also didn't fix any existing warning from the `canary` branch. Would you like me to do that? My preference would be a separate PR to get it in more quickly. Any code changes are commented inline. --------- Co-authored-by: Steven <steven@ceriously.com>
Ping ... hmm, @ijjk? Sorry I don't know who to bug here 😅. But other than some mysterious build failures after merging the latest |
expect(eslintrcJson).toMatchObject({ extends: 'next/core-web-vitals' }) | ||
expect(eslintrcJson).toMatchObject({ | ||
extends: ['next/core-web-vitals', 'next/typescript'], | ||
}) |
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.
Are we missing any tests here?
For example, the linked issue mentions that it catches floating promises so perhaps we need a test for that?
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 don't recall ever seeing a project add explicit tests for particular lint rules being enabled. It's an interesting idea though.
Generally, the presence of something like next/typescript
in the extends
of an ESLint config has been considered enough. This is essentially a hasBeenCalledWith
for the ESLint API.
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.
This looks like it would be a breaking change for next lint
so we'll need to consider this semver-major.
👋 ping @styfle, is there anything I should do to be useful here? |
@@ -7450,6 +7521,25 @@ packages: | |||
- typescript | |||
dev: true | |||
|
|||
/@typescript-eslint/utils@6.14.0(eslint@8.31.0)(typescript@4.8.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.
Note that this is duplicated with a 5.x version of @typescript-eslint/utils
because eslint-plugin-jest
still relies on @typescript-eslint/utils@5
.
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@rushstack/eslint-patch@1.3.3, npm/eslint-import-resolver-node@0.3.6, npm/eslint-import-resolver-typescript@3.5.2, npm/eslint-plugin-import@2.28.1, npm/eslint-plugin-jsx-a11y@6.7.1, npm/eslint-plugin-react-hooks@4.5.0, npm/eslint@8.31.0, npm/kleur@4.1.3, npm/moment@2.24.0, npm/source-map@0.7.3, npm/swr@2.2.4, npm/typescript@4.8.2, npm/webpack-bundle-analyzer@4.6.1, npm/webpack-stats-plugin@1.1.0 |
288262b
to
a509c64
Compare
👋 @styfle / anybody, is there anything you're waiting for on my end to review? Resolving these merge conflicts takes up time for me and I'm unclear on whether & when this will be re-reviewed. Edit: the force push is from me trying to resolve merge conflicts, messing up the |
Last time I used lint rules using typechecking, it made ESLint horribly slow. Is ESLint report still blocked on the slowest rule or is in-editor linting streaming in results as rules are checked? Typechecking performance is a big problem in non-trivial apps. Do you have some linting benchmarks (CLI and in-editor) on non-trivial apps? To reduce the blast radius, we could also make type-checked rules opt-in. |
@eps1lon you're right, ESLint is still blocked on the slowest rule - as (a) it wouldn't be able to produce reports for a file until all the file's rules are done, and (b) some rules spookily modify state depended upon by other rules (😬). typescript-eslint v8 will have a set of performance improvements (typescript-eslint/typescript-eslint#8922) & configuration streamlining (typescript-eslint/typescript-eslint#8031), but [typed linting] will still be orders of magnitude slower than traditional linting. It'll still be blocked on roughly the speed of TypeScript type checking in the common case, assuming the project hasn't misconfigured anything. I do plan on setting up performance comparisons but first up will be getting typescript-eslint@v8 in user testing (typescript-eslint/typescript-eslint#9022). We can expect the comparisons middle to end of this summer, roughly. I like the idea of starting with just the recommended config, and then later considering adding in an opt-in for typed linting. I won't be able to implement it this month but if nobody has updated this PR for me or sent a new one by June, I can. 🙂 |
Fixes #37151.
This differs a bit from the discussed approach for simplicity's sake now that typescript-eslint v6 is out. I started inline comments.This takes the approach suggested in the discussion of adding anext/typescript
linter preset.Also applies a small refactor to how the strict config is found from the prompt values. Instead of a
.find
, I extracted out the specific value into its own function.