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(types): fix types of flat configs#3879
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:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecovbot commentedJan 14, 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 @@## master #3879 +/- ##==========================================+ Coverage 97.66% 97.80% +0.14%========================================== Files 136 136 Lines 9978 9978 Branches 3703 3703 ==========================================+ Hits 9745 9759 +14+ Misses 233 219 -14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
index.js Outdated
@@ -90,14 +90,14 @@ const configs = { | |||
'react/jsx-uses-react': SEVERITY_OFF, | |||
}, | |||
}, | |||
flat: /** @type {Record<string, ReactFlatConfig>} */ ({ | |||
flat: /** @type {Record<"recommended"|"all"|"jsx-runtime", ReactFlatConfig>} */ ({ |
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.
flat:/**@type {Record<"recommended"|"all"|"jsx-runtime", ReactFlatConfig>} */({ | |
flat:/**@type {Record<'recommended' | 'all' | 'jsx-runtime', ReactFlatConfig>} */({ |
we should probably define this union as its own type.
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 did some refactoring on the types, but I haven't figured out a good way to typeflat
first and assign its value later. Maybe using dummy values is an option but I haven't tried to construct such a complex object yet, and type assertion on an empty object is not available in js file.
Are you open to move flat config to a separate object instead of nesting it inside the originalconfigs
object? However this is a breaking change and I do want to avoid it if there are other options. And just curious, what was the reason that the new flat config not being added as a new object?
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 just meant,/** @typedef {'recommended' | 'all' | 'jsx-runtime'} AvailableFlatConfigs */
or similar.
A breaking change is not just something to avoid, it's simply not an option. Flat configs are nested by design and that will remain the case.
I'm not sure what you mean by "being added as a new object" - the "configs" key is the reasonable place to put all configs, flat or not.
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 just meant, /**@typedef {'recommended' | 'all' | 'jsx-runtime'} AvailableFlatConfigs */ or similar.
Yeah I get that, but I got some errors while runningnpm run build-types
and started to type things more explicitly than usingtypeof
, now I have switched back to the minimum changes as I did at the beginning and try to make it work. So far typing the key asAvailableFlatConfigs
makeconfigs
withflat
incompatible with theeslintconfigs
type in the test-published-types step, but the package itself builds and packs without problem.
What I meant by adding a new object is thatflatConfig
itself being an individual object and can be accessed throughreactPlugin.flatConfigs
, rather than putting it inside a legacy configs object. Some plugins did it that way such aseslint-plugin-jsx-a11y. I had this thought as I was trying to avoid the circular type reference on the legacy config and the flat 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.
Now I have a better understanding of the problem: In thetest-published-types test,react
is supposed to beRecord<string, ESLint.Plugin>
as definedhere, thelegacy configs inconfigs
fits that signature, but the newflat
config object we put inside doesn't, this is another reason to not nested the flat config inside the legacy config object.
Maybe there is a hacky way to bypass/force this to work? Otherwise I am not sure how to type the flat config object properly while keeping the current placement, which on the one hand is a reasonable place to put configs, on the other hand doesn't comply with 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.
Instead of Record, try{ [k in AvailableFlatConfigs]: ReactFlatConfig }
?
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 doesn't work, the problem is notAvailableFlatConfigs
not compatible withstring
(which I misunderstood in#3879 (comment)).
The problem isRecord<AvailableFlatConfigs, ReactFlatConfig>
or{ [k in AvailableFlatConfigs]: ReactFlatConfig }
is not compatible withESLint.Plugin
, i.e. theconfigs object in the code, which was a legacy config and used in a way liketest-published-types, shouldn't include a nonESLint.Plugin
property (flat
in this case).
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.
ESLint's own types don't dictate what we do - only its runtime behavior does. If there's something the runtime allows that the types do not, it's a bug in their types, and it should be filed as such.
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.
Thanks, I have updated the PR with my latest changes for future reference, it still has the typing issue but I am not sure how to proceed. There could be a few options based on our conversations so far:
- Ask ESLint to loose their type definition for the
plugins
inConfig
to takeRecord<string, any>
so that plugins can put any object into the value. Personally I don't think this make sense from ESLint's typing and design perspective even though you explained about only the runtime behavior matters - Make the type of the React plugin comply with the ESLint's expectation. This could be one of the following:
- Put legacy config into its own object and use
configs.legacy
orconfigs.flat
explicitlyconfigs={legacy:{...}flat:{...}}
- Move flat config into a separate object
configs={...}// legacyflatConfigs={...}}
- Put legacy config into its own object and use
- Merge the change as is and updatetest-published-types somehow to allow this. If test-published-types doesn't reflect what the actual user does, test it this way doesn't help. If it does, either 1 or 2 should happen.
Please feel free to share other potential options or take over the PR, I am just going to share a workaround in the original issue and move forward unless there is a viable option at the moment.
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.
To test the typing issue locally
//in eslint-plugin-reactnpm pack --pack-destination /tmp///in eslint-plugin-react/test-published-types// Use any TS versionin https://github.com/jsx-eslint/eslint-plugin-react/actions/runs/12776433146/job/35615079535?pr=3879npm installnpm install --no-save /tmp/eslint-plugin-react-7.37.4.tgz typescript@4.0// Use any lib versionin the action abovenpx tsc --lib es2015
cd30752
tob7ad705
Compareb7ad705
toc51432e
Compare
Fixes#3878