react
react copied to clipboard
facebook
feat(eslint-plugin-react-hooks): support flat config
I also updated the README to include usage examples.
Example usage:
import reactHooks from 'eslint-plugin-react-hooks';
export default [
{
files: ['**/*.{js,jsx}'],
languageOptions: {
ecmaVersion: 2020,
globals: globals.browser,
parserOptions: {
ecmaVersion: 'latest',
ecmaFeatures: {jsx: true},
sourceType: 'module',
},
},
settings: {react: {version: '18.3'}},
...reactHooks.configs['recommended-latest'],
},
];
Closes #28313
Hi @michaelfaith!
Thank you for your pull request and welcome to our community.
Action Required
In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.
Process
In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.
Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.
If you have received this in error or have any questions, please contact us at [email protected]. Thanks!
The latest updates on your projects. Learn more about Vercel for Git ↗︎
| Name | Status | Preview | Comments | Updated (UTC) |
|---|---|---|---|---|
| react-compiler-playground | ✅ Ready (Inspect) | Visit Preview | 💬 Add feedback | Oct 30, 2024 8:52am |
![vercel[bot] avatar](https://avatars.githubusercontent.com/in/8329?v=4 "Published by a pub.dev verified publisher"){kind=small}
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!
Hi, any advance with this?
Not sure. Waiting on someone from the team to look at it.
@eps1lon with the release of 5.0 and eslint v9 support 🥳 , would it be possible to push this forward now for flat config support? Any feedback would be greatly appreciated.
Can you rebase? Seems like CI missed this.
Done. And made the adjustment to the index, to remove the new dependency on the rollup plugin.
The support for flat config is backwards compatible with ESLint v8 and lower?
Comparing: 1185f88d35e83870aa220a30ddcec22ed7e8e362...1a4ee0f0197542a3fba282ba34237939d3e6e737
Critical size changes
Includes critical production bundles, as well as any change greater than 2%:
Significant size changes
Includes any change greater than 0.2%:
Expand to show
| Name | +/- | Base | Current | +/- gzip | Base gzip | Current gzip |
|---|---|---|---|---|---|---|
| oss-experimental/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js | +5.17% | 89.94 kB | 94.58 kB | +2.41% | 14.97 kB | 15.33 kB |
| oss-stable-semver/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js | +5.15% | 87.86 kB | 92.39 kB | +2.38% | 14.73 kB | 15.08 kB |
| oss-stable/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js | +5.15% | 87.86 kB | 92.39 kB | +2.38% | 14.73 kB | 15.08 kB |
| oss-stable-semver/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.js | +5.14% | 78.52 kB | 82.55 kB | +2.31% | 14.37 kB | 14.71 kB |
| oss-stable/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.js | +5.14% | 78.52 kB | 82.55 kB | +2.31% | 14.37 kB | 14.71 kB |
| oss-experimental/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.js | +5.13% | 80.38 kB | 84.50 kB | +2.36% | 14.63 kB | 14.98 kB |
Generated by :no_entry_sign: dangerJS against 4b0aab596279592d6b4fab6d3b43196915bb339c
The support for flat config is backwards compatible with ESLint v8 and lower?
Yep! This shouldn't impact any earlier versions. The "legacy" configs are exported from the same place as before (configs), while the new flat configs are on a new object flatConfigs. Older configs (rc-based), will just be looking for the presence of the configs export, and won't care or notice that a new export is present, while flat config users will just interact with the new export. The new meta export is needed for flat config support, but won't interfere with older versions.
the new flat configs are on a new object
flatConfigs
🚨 WARNING 🚨
Putting flat configs on a new flatConfig field will make ESLint type definitions throw errors because that does not follow the correct ESLint plugin shape. This will disrupt some users with TypeScript and force them to use type assertions and other workarounds. eslint-plugin-react has a similar issue, so hopefully this plugin can avoid that.
ESLint wants flat configs to be included in the configs field, alongside legacy configs.
// https://github.com/eslint/eslint/blob/main/lib/types/index.d.ts#L1455
interface Plugin extends ObjectMetaProperties {
configs?: Record<string, ConfigData | Linter.FlatConfig | Linter.FlatConfig[]> | undefined;
...
}
I've seen other plugins (example @stylistic/eslint-plugin) solve this by creating new "*-flat" configs (example: recommended-flat). The legacy configs can still be used without disruption.
Eventually, when ESLint v10 removes support for legacy configs, these flat config names can be turned into aliases and deprecated.
@JstnMcBrd eslint really doesn't get to decide the export layout for other plugins, so if that's an issue with the types then that should be fixed there.
@ljharb Hey Jordan. I understand as maintainer of eslint-plugin-react, you prefer your export layout, but I must disagree here. ESLint does determine the expected layout for ESLint plugins. It's possible for configs to stray outside that layout without runtime issues, but we can't expect the types to change for every possible custom layout developers come up with!
Following the layout ESLint asks for doesn't cost much, and it reduces inconvenience for TS users (including me 😅). Sometimes there are extenuating circumstances that outweigh this, but I don't think that's the case here.
@JstnMcBrd the types shouldn't be so prescriptive, especially since the runtime allows it just fine. If eslint wants to enforce it, then the actual code that matters - the runtime javascript - needs to enforce it, and the types need to match what the code does and no more.
I've seen other plugins (example
@stylistic/eslint-plugin) solve this by creating new"*-flat"configs (example:recommended-flat). The legacy configs can still be used without disruption.
I haven't seen that convention. But I have seen 'flat/*' a lot. e.g. configs['flat/recommended']. Honestly, I don't have a strong opinion either way. There's a lot of inconsistency across the community for how new flat configs have been exported. I think the inconsistency across plugins is a bigger rough edge than any one of these options on their own.
Eventually, when ESLint v10 removes support for legacy configs, these flat config names can be turned into aliases and deprecated.
The same holds true for any of these conventions.
I would be comfortable with either flatConfigs.recommended (what it currently is and what feels a bit more ergonomic to me) or configs['flat/recommended'], since those seem to be the two leading conventions I've seen emerge. Whatever direction @eps1lon wants to go.
the types shouldn't be so prescriptive, especially since the runtime allows it just fine. the types need to match what the code does and no more
I agree - types should always match what the code does. But there's nuance here. I think there's a good reason the types are this way. It's not super relevant to this PR, but the reasoning is below for anyone interested.
How I understand it
Yes, when you pass a plugin to ESLint at runtime, it does not care what plugin.configs is. If the types represented that, they would just be configs: any.
BUT, when a user pulls a config from your plugin and gives it to ESLint, it definitely cares what that is (see the types for LegacyConfig and Config).
ESLint needs to define plugin.configs to help devs know if they make an incorrect config. Not because the plugin will blow up at runtime, but because the config will, if a user tries to use it. If the type was configs: any, there would be no warning. The LegacyConfig and Config types would not get enforced.
Okay, so the type is useful, but why so prescriptive? Well, there are unlimited ways developers can export their configs on the plugin, which is impossible to type well. Anything is prescriptive compared to complete permissiveness.
A circular type would be necessary to represent the nesting on eslint-plugin-react - not impossible, but not ideal. But to represent the flatConfigs object in this PR, ESLint would need to assert arbitrary fields on the plugin are config objects. And that would conflict with plugins that have custom fields with other types. And I'm sure there are more ways.
So yes, the plugin.configs type is prescriptive, but that's necessary to be useful and define what the runtime allows when those configs are used.
It does introduce a convention, but it's fairly easy to follow it from the start (which I hope we do here).
Given that eslint-plugin-import and eslint-plugin-jsx-a11y and eslint-config-airbnb are going to match eslint-plugin-react, eslint can choose to update their types or not. ¯\_(ツ)_/¯
@JstnMcBrd shouldn't the types for this package come from a @types/eslint-plugin-react-hooks package, in which case, the new export could just be added, in the same way that it was for @types/eslint-plugin-jsx-ally https://www.npmjs.com/package/@types/eslint-plugin-jsx-a11y?
@ljharb's right that types shouldn't dictate runtime, but the other way around. And I would actually argue that this approach gives you even richer type information, and a cleaner deprecation / removal path.
What @JstnMcBrd said above about the types resonates with me.
Here's my understanding of ESLint config types:
- ESLint consumes configuration from the user, including plugins
- ESLint expects this config, including plugins, to be a certain shape and match a contract (with TS types via community-contributed DefinitelyTyped
@types/eslintand in future ESLint built-in types)
Both of these are pretty common in TS configuration patterns across the ecosystem.
It feels like, given these constraints, plugins which don't conform to these types are publishing incorrect code.
But maybe it would be good to CC some of the folks involved in @types/eslint (DefinitelyTyped), ESLint and typescript-eslint to get some other opinions:
- ESLint
- @nzakas
@types/eslint- @antfu, @jakebailey, @JounQin, @linlinyang, @matwilko
typescript-eslint- @bradzacher, @JoshuaKGoldberg
@types/eslint will be deprecated and unneeded soon.
https://github.com/DefinitelyTyped/DefinitelyTyped/pull/70735
https://github.com/eslint/eslint/issues/18928
Oh interesting, then the question of the strictness / interface of the ESLint plugin types is no longer a question for the @types/eslint DefinitelyTyped community maintainers, more of a question for the ESLint team.
Sorry, I don't have time to review the entire thread here. Can someone please summarize what the question is that I was mentioned to help answer?
@nzakas i believe the question is whether (from ESLint’s perspective) a plugin must export all configs under plugin.configs, or whether it’s acceptable to export both a plugin.configs and plugin.flatConfigs objects. @JstnMcBrd believes the type definitions from @types/eslint imply a structural constraint on the Plugin interface that would be violated by using plugin.flatConfigs.
basically: does ESLint have a stance on plugin.configs['flat/recommended'] vs plugin.flatConfigs.recommended?
Got it, thanks.
So the recommendation is that all configs, regardless of being eslintrc or flat, should be on the configs object.
We also recommend that you not include the word "flat" in the config name. We recommend that flat configs just include the name (for example, recommended) and any eslintrc-style configs append -legacy to the name, such as recommended-legacy. (Larger discussion around this here: https://github.com/eslint/eslint/issues/18095#issuecomment-1952981425)
and any eslintrc-style configs append
-legacyto the name, such asrecommended-legacy.
That would be a breaking change though. And this plugin just cut a major release. So, to introduce another breaking change on the heals of that, just to add "-legacy" to the existing config name, doesn't seem prudent...