fix(types): fix types of flat configs

[CHC383]

Fixes #3878

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.80%. Comparing base (e6b5b41) to head (2236728).

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.
📢 Have feedback on the report? Share it here.

[ljharb]

Suggested change

	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.

[CHC383]

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?

[ljharb]

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.

[CHC383]

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

[CHC383]

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.

[ljharb]

Instead of Record, try { [k in AvailableFlatConfigs]: ReactFlatConfig }?

[CHC383]

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).

[ljharb]

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.

[CHC383]

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:

1. Ask ESLint to loose their type definition for the plugins in Config to take Record<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
2. 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 or configs.flat explicitly

     configs = {
         legacy: {...}
         flat: {...}
     }

   • Move flat config into a separate object

     configs = {...} // legacy
     flatConfigs = {...}
     }

   However, both of them are breaking changes. Even though I don't like them as much as you do, I am not sure if putting the new flat config into the legacy config was a right decision that led to the situation today. Both eslint-plugin-import and eslint-plugin-jsx-a11y don't nest the flat configs in this way
3. 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.

[CHC383]

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