-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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(types): fix types of flat configs #3879
base: master
Are you sure you want to change the base?
Conversation
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. |
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 type flat
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 original configs
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 running npm run build-types
and started to type things more explicitly than using typeof
, 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 as AvailableFlatConfigs
make configs
with flat
incompatible with the eslint configs
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 that flatConfig
itself being an individual object and can be accessed through reactPlugin.flatConfigs
, rather than putting it inside a legacy configs object. Some plugins did it that way such as eslint-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 the test-published-types test, react
is supposed to be Record<string, ESLint.Plugin>
as defined here, the legacy configs in configs
fits that signature, but the new flat
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 not AvailableFlatConfigs
not compatible with string
(which I misunderstood in #3879 (comment)).
The problem is Record<AvailableFlatConfigs, ReactFlatConfig>
or { [k in AvailableFlatConfigs]: ReactFlatConfig }
is not compatible with ESLint.Plugin
, i.e. the configs object in the code, which was a legacy config and used in a way like test-published-types, shouldn't include a non ESLint.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 = {...} // legacy flatConfigs = {...} }
- Put legacy config into its own object and use
- Merge the change as is and update test-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-react
npm pack --pack-destination /tmp/
// in eslint-plugin-react/test-published-types
// Use any TS version in https://github.com/jsx-eslint/eslint-plugin-react/actions/runs/12776433146/job/35615079535?pr=3879
npm install
npm install --no-save /tmp/eslint-plugin-react-7.37.4.tgz [email protected]
// Use any lib version in the action above
npx tsc --lib es2015
cd30752
to
b7ad705
Compare
b7ad705
to
c51432e
Compare
Fixes #3878