jsx-eslint / eslint-plugin-react

React-specific linting rules for ESLint
MIT License
8.86k stars 2.76k forks source link

[Fix] export flat configs from plugin root and fix flat config crash #3694

Open bradzacher opened 4 months ago

bradzacher commented 4 months ago

fixes #3693

This PR exposes the flat configs at the root of the plugin:

const reactPlugin = require('eslint-plugin-react');

module.exports = [
  // ...
  reactPlugin.configs['flat/recommended'],
  // ...
];

The flat/ prefix is an approach a number of plugins are taking to enable supporting both styles at the top-level.


This was the easiest way to set things up so that the plugin reference was consistent across the root and all configs.

I'm open to a different approach to fixing the issue - for example if you want to ensure the config/ exports all work as well - it's just going to be a much larger refactor to make that work because we need to restructure the configs so there isn't a cyclic dependency between the configs and the plugin.

Personally I'm of the opinion that this "everything from the root" approach offers better DevX compared to forcing users to separately import the configs. It's a stylistic thing though - up to you really.

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.76%. Comparing base (393bfa2) to head (eb0c123).

:exclamation: Current head eb0c123 differs from pull request most recent head a44e025

Please upload reports for the commit a44e025 to get more accurate results.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #3694 +/- ## ========================================== + Coverage 94.28% 97.76% +3.47% ========================================== Files 134 133 -1 Lines 9613 9475 -138 Branches 3486 3472 -14 ========================================== + Hits 9064 9263 +199 + Misses 549 212 -337 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ljharb commented 4 months ago

hmm - i definitely prefer deep imports; barrel files/manifest exports cause all sorts of problems.

how would the refactor work to have flat configs not be at the root?

bradzacher commented 4 months ago

how would the refactor work to have flat configs not be at the root?

An example of a plugin that goes your deep import approach is https://github.com/eslint-community/eslint-plugin-eslint-plugin/

They define the flat configs as a function of the legacy configs.

EG

const plugin = require('../index');

module.exports = {
  plugins: { react: plugin },
  rules: plugin.configs.recommended.rules,
};

But their usecase is a bit simpler as it doesn't need to worry about language options etc.

ljharb commented 4 months ago

I've rebased this; i'm still hopeful for a way to have the root be the current legacy config, in particular to avoid a breaking change.

jakec-dev commented 2 months ago

Any update on this? This plugin is unusable with Airbnb config until this is pulled

ljharb commented 2 months ago

@jakec-dev the airbnb config doesn't use flat config, so it should be perfectly usable if you use a compatible version of eslint (not 9, yet) and the default config format of the supported eslint versions (eslintrc)

mdjermanovic commented 2 weeks ago

I've rebased this; i'm still hopeful for a way to have the root be the current legacy config, in particular to avoid a breaking change.

It isn't a problem for either of the two config systems (eslintrc and flat) to include both config formats in configs, with different keys of course. Eslintrc users would keep using the same configs (no breaking changes) and flat config users would be using the new configs (reactPlugin.configs['flat/recommended'] as in the original post).

mdjermanovic commented 2 weeks ago

On the other hand, if you prefer deep imports, a solution could be to invert https://github.com/jsx-eslint/eslint-plugin-react/commit/17858beeedaaf5d0eb4fd1cc292fb34d07f9f659: move rule configs and parserOptions back to index.js and update configs/recommended.js and others to load index.js and translate configs into the flat config format.

mdjermanovic commented 2 weeks ago

Also relevant: https://github.com/eslint/eslint/issues/18095

ljharb commented 2 weeks ago

I vastly prefer deep imports, for both eslintrc and flat.

If we can use those, and ensure no breakage for existing eslintrc users, then that sounds like a great solution to get this over the line.

mdjermanovic commented 2 weeks ago

I vastly prefer deep imports, for both eslintrc and flat.

If we can use those, and ensure no breakage for existing eslintrc users, then that sounds like a great solution to get this over the line.

eslintrc config system doesn't provide a way for plugins to export configs other than under the configs key, so there's probably no point in adding new (deep) exports for eslintrc configs.

As for flat configs, this is doable as @bradzacher suggested in https://github.com/jsx-eslint/eslint-plugin-react/pull/3694#issuecomment-1958261297. Technically, it could be implemented as suggested in https://github.com/jsx-eslint/eslint-plugin-react/pull/3694#issuecomment-2163881696.

ljharb commented 2 weeks ago

Sounds great, let's do that :-)

If you drop a link to a sha or branch here, i can pull it into this PR

mdjermanovic commented 2 weeks ago

Here's the branch:

https://github.com/mdjermanovic/eslint-plugin-react/tree/fix-flat-configs

I'm not sure why languageOptions in configs were set non-enumerable, but changing that would probably be a breaking change.

ljharb commented 2 weeks ago

It was set that way because eslint threw when it was enumerable - i forget whether it threw with eslintrc, or with flat config, but making it nonenumerable hid it from one and allowed the other to read it.

ljharb commented 2 weeks ago

@mdjermanovic hm, those two commits are on top of master, not this PR, and there's a conflict. I've rebased this PR; can you stack your commits on top of it, and then I can pull them in?

mdjermanovic commented 2 weeks ago

@ljharb to clarify, do you want eslint-plugin-react to provide two ways to use its flat configs?

1st:

const reactRecommended = require('eslint-plugin-react/configs/recommended');

module.exports = [
  reactRecommended
];

2nd:

const reactPlugin = require('eslint-plugin-react');

module.exports = [
  reactPlugin.configs['flat/recommended']
];
ljharb commented 2 weeks ago

I’m comfortable with that, as long as it doesn’t introduce cycles - but it’d also be sufficient to only have the deep import way (but it should say “flat” somewhere in there)

mdjermanovic commented 2 weeks ago

Here's a branch on top of this PR. It keeps flat configs in plugin root (those added in this PR), fixes existing flat configs intended for deep imports, and adds tests for both:

https://github.com/mdjermanovic/eslint-plugin-react/tree/fix-deep-flat-configs