Skip to content
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

feat: support new config system #891

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jjangga0214
Copy link

@jjangga0214 jjangga0214 commented Sep 13, 2022

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.

 - add top-level entry points for recommended configs
 - add documentation to readme
@codecov
Copy link

codecov bot commented Sep 14, 2022

Codecov Report

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

❗ 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

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that the shared configs provided by this plugin need to be in their own entrypoints to work with the new system.

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?

package.json Outdated Show resolved Hide resolved
@jjangga0214
Copy link
Author

jjangga0214 commented Sep 14, 2022

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.

@ljharb
Copy link
Member

ljharb commented Sep 14, 2022

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.

@jjangga0214
Copy link
Author

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
Copy link
Author

Pushed a new commit :)

@ljharb

This comment was marked as resolved.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed up the code samples, added the missing mjs/cjs extensions (native ESM should always be .mjs, not .js, so it's critical to allow that), and converted the CJS files you added in src/ to ESM.

I'm dismayed that eslint's new config system removes the env key; users shouldn't have to import globals themselves, especially if they'd have to depend directly on the globals package.

@ljharb ljharb force-pushed the feat/new-config-system branch 2 times, most recently from 77a0186 to a6750f7 Compare September 15, 2022 17:34
@ljharb
Copy link
Member

ljharb commented Sep 15, 2022

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?

@jjangga0214
Copy link
Author

jjangga0214 commented Sep 16, 2022

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?

@ljharb
Copy link
Member

ljharb commented Sep 16, 2022

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.

@jjangga0214
Copy link
Author

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?

@ljharb
Copy link
Member

ljharb commented Sep 27, 2022

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

@jjangga0214
Copy link
Author

jjangga0214 commented Sep 28, 2022

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.

@ljharb
Copy link
Member

ljharb commented Sep 28, 2022

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

@jjangga0214
Copy link
Author

jjangga0214 commented Sep 28, 2022

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?

@ljharb
Copy link
Member

ljharb commented Aug 12, 2023

@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 :-)

@pauliesnug
Copy link

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

@ljharb
Copy link
Member

ljharb commented Dec 19, 2023

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

@jjangga0214
Copy link
Author

jjangga0214 commented Dec 19, 2023

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

plugins: [
'jsx-a11y',
],
parserOptions: {
ecmaFeatures: {
jsx: true,
},
},

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

@ljharb
Copy link
Member

ljharb commented Dec 19, 2023

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

@anselmbradford
Copy link

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

@hanselabreu
Copy link

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

@soerenmartius
Copy link

soerenmartius commented Mar 18, 2024

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

@ljharb
Copy link
Member

ljharb commented Apr 8, 2024

@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 ljharb mentioned this pull request Apr 8, 2024
9 tasks
@soerenmartius
Copy link

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for ESLint 9
6 participants