eslint-plugin-jsx-a11y icon indicating copy to clipboard operation
eslint-plugin-jsx-a11y copied to clipboard

feat: support new config system

Open jjangga0214 opened this issue 3 years ago • 24 comments

Supporting eslint's new config system of eslint.

Note that the legacy config system always has require()d plugins and sharable configs, whereas the new system is available with both of ESM and CJS.

Thus conditional export is introduced to keep compatibility while providing a new config/plugin at the same time.

plugin: protocol(e.g. plugin:react-hooks/recommended) is not valid any more. Thus the new plugin doesn't have configs.

Fixes #978.

jjangga0214 avatar Sep 13 '22 07:09 jjangga0214

Codecov Report

Merging #891 (fe5bb49) into main (7b3492b) will increase coverage by 0.00%. The diff coverage is n/a.

:exclamation: Current head fe5bb49 differs from pull request most recent head 3e48857. Consider uploading reports for the commit 3e48857 to get more accurate results

@@           Coverage Diff           @@
##             main     #891   +/-   ##
=======================================
  Coverage   99.29%   99.29%           
=======================================
  Files         104      103    -1     
  Lines        1555     1571   +16     
  Branches      523      514    -9     
=======================================
+ Hits         1544     1560   +16     
  Misses         11       11           

see 24 files with indirect coverage changes

codecov[bot] avatar Sep 14 '22 06:09 codecov[bot]

However, why can't the existing "main" Just Work with the new config system? What's in legacy.js that's different from index.js?

I have never said the existing "main" just doesn't work. It works because the only difference is the existence of configs property, and the new config system would ignore it when the plugin object still has it. But my point was that it's not the intended specification, and can confuse users, leading to weird usage.

jjangga0214 avatar Sep 14 '22 06:09 jjangga0214

I'm not sure why a user would be confused, and if they are, I think that's on eslint to fix, not individual plugins in the ecosystem.

ljharb avatar Sep 14 '22 07:09 ljharb

I'm not sure why a user would be confused, and if they are, I think that's on eslint to fix, not individual plugins in the ecosystem.

Okay. I respect that. I'd patch as long as this is resolved.

jjangga0214 avatar Sep 14 '22 07:09 jjangga0214

Pushed a new commit :)

jjangga0214 avatar Sep 15 '22 06:09 jjangga0214

why do you need to bring in your own globals? isn't that what env is for?

ljharb avatar Sep 15 '22 16:09 ljharb

hmm, i'm having second thoughts on the files config. @jjangga0214, if files: ['**/*.{js,mjs,cjs,jsx,mjsx,ts,tsx,mtsx}'] is included by default in the legacy config system, will that break anything for them?

ljharb avatar Sep 15 '22 17:09 ljharb

if files: ['**/*.{js,mjs,cjs,jsx,mjsx,ts,tsx,mtsx}'] is included by default in the legacy config system, will that break anything for them?

Well, my opinion is yes, if so. But we know the assumption is not true, so what did make you think about the imagination?

jjangga0214 avatar Sep 16 '22 02:09 jjangga0214

I didn't understand your reply.

What I realized is that it's critical for all of these extensions to be included, and highly unlikely devs will remember to include them all, so it's valuable to provide them by default. However, I want the plugin object to be able to be usable in both config systems.

ljharb avatar Sep 16 '22 04:09 ljharb

It seems there's confusion between us?

if files: ['**/*.{js,mjs,cjs,jsx,mjsx,ts,tsx,mtsx}'] is included by default in the legacy config system,

To my understanding, files: ['**/*.{js,mjs,cjs,jsx,mjsx,ts,tsx,mtsx}'] is NOT included by default in the legacy config system. Am I wrong?

jjangga0214 avatar Sep 26 '22 10:09 jjangga0214

@jjangga0214 what i mean is, in the legacy config, is the presence of a "files" property problematic? or is it ignored?

ljharb avatar Sep 27 '22 16:09 ljharb

I don't understand the point. Legacy config should not be used in the new config. They're just different and incompatible. The exception is when the legacy config only has .rules. Other fields like .plugins(conflict. Legacy: string[], New: PluginObject[]),.extends(ignored), .overrides (ignored), .parser (ignored), .parserOptions (ignored), .env (ignored) and more, are incompatible. For almost all plugins in the ecosystem, including eslint-plugin-jsx-a11y, their presets have at least .plugins or .extends.

So I still don't grasp what your concern is. I think I may understand if you show me an example of your thoughts.

jjangga0214 avatar Sep 28 '22 02:09 jjangga0214

"Different and incompatible" is what I'm trying to confirm.

If in fact the same object can't serve in both configs, then that's a massive design mistake on eslint's side - and I'd prefer to impose the cost on the users of the new config, since the legacy config is the most important (and will remain so for a good number of years).

ljharb avatar Sep 28 '22 05:09 ljharb

I'd prefer to impose the cost on the users of the new config

Does this mean you're not supposed to support the new config system, which also means you'd not merge this PR?

jjangga0214 avatar Sep 28 '22 05:09 jjangga0214

@jjangga0214 oh no, not at all! it just means the main entrypoint of the package will be the legacy config, and flat config users will need to import, eg, eslint-plugin-jsx-a11y/flat or something.

However, if the same object can be used in both config systems, then the main entrypoint can support both, which is the ideal.

I've just rebased this, and if tests pass, and we can confirm that the main entrypoint works in both configs, then it'd be good to go :-)

ljharb avatar Aug 12 '23 05:08 ljharb

@ljharb is this being maintained? the last update was four months ago when you said it would be merged

pauliesnug avatar Dec 19 '23 05:12 pauliesnug

@pauliesnug of course it is. I'm waiting for confirmation that the plugin works in both config formats.

ljharb avatar Dec 19 '23 05:12 ljharb

@ljharb @pauliesnug

oh no, not at all! it just means the main entrypoint of the package will be the legacy config, and flat config users will need to import, eg, eslint-plugin-jsx-a11y/flat or something.

Okay, I now understand it.

if the same object can be used in both config systems,

Unfortunately, no.

https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/1adec3517fc2c9797212ca4d38858deed917e7be/src/index.js#L47-L54

For example, as I've said before, plugins as string[] is incompatible, and parserOptions should be under languageOptions. The only compatible property is rules.

Note that the spec of plugin itself(not config) is compatible, by the way.

So in conclusion, we should provide a different object by a different entry point. Any idea about the name of it? (You already mentioned eslint-plugin-jsx-a11y/flat. Is it a suggestion or just an example?)

jjangga0214 avatar Dec 19 '23 06:12 jjangga0214

Just an example, but yes, it'd need to be some other entry point.

ljharb avatar Dec 19 '23 06:12 ljharb

Any idea about the name of it?

FWIW 'flat/recommended' is what eslint-plugin-jsdoc names it https://www.npmjs.com/package/eslint-plugin-jsdoc#flat-config

anselmbradford avatar Feb 08 '24 21:02 anselmbradford

+1 on this. This would be useful for importing recommended/strict settings via CLI (e.g. eslint --config node_modules/eslint-plugin-jsx-a11y/recommended.js). Which is currently impossible since the current index.js contains additional configurations.

hanselabreu avatar Feb 14 '24 00:02 hanselabreu

Is this still being worked on? Otherwise I can always start a PR

soerenmartius avatar Mar 18 '24 07:03 soerenmartius

@soerenmartius please don't open a new PR; if you want to rebase this PR and keep working on it, please comment a branch link and i'll pull in your changes.

ljharb avatar Apr 08 '24 04:04 ljharb

@soerenmartius please don't open a new PR; if you want to rebase this PR and keep working on it, please comment a branch link and i'll pull in your changes.

Thanks - I am on PTO this and next week but will try to contribute once I am back unless you have time to finish this until then! Thanks

soerenmartius avatar Apr 08 '24 04:04 soerenmartius

Is this still being worked on? I might be able to help out, if nobody's working on it. Really eager for the v9 & flat config support.

michaelfaith avatar Jun 15 '24 19:06 michaelfaith

@michaelfaith if you have updates, please comment a link to a branch or sha (not a new PR) and i'll pull in the changes.

ljharb avatar Jun 15 '24 20:06 ljharb

@ljharb I know you mentioned not creating a new PR, but I went in a totally different direction with this, in order to address some of your concerns, so I opened a new PR: https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/pull/993

Totally cool if you'd rather bring those changes into this PR, but figured I'd leave that up to you. (But note, this PR shouldn't close the v9 issue, since it's just focused on flat config support). With my changes, we can use the same main entry point for both legacy and flat experiences, without the need for a separate entry point. This was inspired by how the jsdoc and a couple of other plugins have provided support for both. I also added a new examples folder in the root that has three different (vite-based) React apps using the built plugin, testing usage in both legacy (eslintrc) and flat (esm and cjs) configs.

michaelfaith avatar Jun 18 '24 10:06 michaelfaith

Closing in favor of #993.

ljharb avatar Jun 19 '24 22:06 ljharb