Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
fix(typescript-eslint): make exported configs/plugin compatible withdefineConfig()#11190
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
fix(typescript-eslint): make exported configs/plugin compatible withdefineConfig()#11190
Conversation
Thanks for the PR,@kirkwaiblinger! 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 onhttps://opencollective.com/typescript-eslint. |
netlifybot commentedMay 8, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
✅ Deploy Preview fortypescript-eslint ready!
To edit notification comments on pull requests, go to yourNetlify project configuration. |
nx-cloudbot commentedMay 8, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
defineConfig()defineConfig()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.
looks like this change is a net-positive and removes a bunch of errors from the integration tests -- so don't forget to regen the snapshots.
| /* eslint-disable @typescript-eslint/no-unnecessary-type-assertion -- These "unnecessary" assertions work around "requires explicit type annotation" 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.
could this be done asconst configs = { ... } satisfies Record<string, eslintConfigHelpers.Config | eslintConfigHelpers.Config[]>?
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.
Hmm, well, now I can no longer repro the "requires a type assertion" error at all, maybe because I ended up adding the eslint/config-helpers package in the package.json.
So looks like thesatisfies option is available to us (as well as justconst configs = { ... }). I'll switch it tosatisfies.
| }; | ||
| /* eslint-enable @typescript-eslint/no-unnecessary-type-assertion */ | ||
| export type Config = TSESLint.FlatConfig.ConfigFile; |
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 can't remember -- is this the loose type or the strict type?
Perhaps we should deprecate this to rename it? Or change the type to the ESLint types?
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 is a loose one; it's just aT | Promise<T> version of the type of the parameter type oftseslint.config(...configs).
I think deprecating makes sense 👍
| @@ -51,6 +51,7 @@ | |||
| "check-types": "npx nx typecheck" | |||
| }, | |||
| "dependencies": { | |||
| "@eslint/config-helpers": "^0.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.
ugh an0.x version sucks cos it means that every minor release we'll be forced to update our version or else users will be blocked.
ESLint itself depends on this package so in some future when we drop v8 and older v9 versions -- we would be able to make this a peer dependency and just rely on the version installed that way.
Uh oh!
There was an error while loading.Please reload this page.
chore(typescript-eslint): add type tests for `defineConfig`
codecovbot commentedMay 16, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@## main #11190 +/- ##======================================= Coverage 90.92% 90.92% ======================================= Files 499 499 Lines 50845 50845 Branches 8384 8384 ======================================= Hits 46231 46231 Misses 4599 4599 Partials 15 15
Flags with carried forward coverage won't be shown.Click here to find out more. 🚀 New features to boost your workflow:
|
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.
Looks great! 👍
ah, wait, redrafting since I forgot about the integration test failures |
Uh oh!
There was an error while loading.Please reload this page.
Warning
I'll be travelling until May 27th, and don't expect to be able to spend time on this until then. If someone else is able to resolve the remaining failing integration tests in my absence and get this over the finish line, please feel free to do so!
PR Checklist
defineConfig()types #10899Overview
This switches our exports to use
defineConfig()-compatible types, rather than making our type declarations compatible with eslint core's. The type declarations that we have for things likeFlatConfig.Configand such are deliberately wide/loose, since they're meant to be used as a highly permissiveparameter type fortseslint.config(). So we don't want to be exporting our configs as those types anymore.