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

[New] support new config system #3429

Merged
merged 1 commit into from Oct 10, 2022

Conversation

jjangga0214
Copy link
Contributor

@jjangga0214 jjangga0214 commented Sep 10, 2022

Supporting eslint's new config system of eslint.

Note that the legacy config system always has require()d plugins and sharable configs, while the new system is ESM.

Thus conditional export is great to keep compatibility.

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

Sharable configs for the new config system are now exported as eslint-plugin-react/all, eslint-plugin-react/recommended, eslint-plugin-react/jsx-runtime

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

codecov bot commented Sep 10, 2022

Codecov Report

Merging #3429 (6da5609) into master (feae3bc) will increase coverage by 0.04%.
The diff coverage is n/a.

❗ Current head 6da5609 differs from pull request most recent head 17858be. Consider uploading reports for the commit 17858be to get more accurate results

@@            Coverage Diff             @@
##           master    #3429      +/-   ##
==========================================
+ Coverage   97.58%   97.62%   +0.04%     
==========================================
  Files         124      123       -1     
  Lines        9055     8959      -96     
  Branches     3308     3272      -36     
==========================================
- Hits         8836     8746      -90     
+ Misses        219      213       -6     
Impacted Files Coverage Δ
lib/rules/jsx-sort-default-props.js 90.00% <0.00%> (-1.05%) ⬇️
lib/rules/jsx-no-constructed-context-values.js 93.13% <0.00%> (-0.14%) ⬇️
lib/rules/jsx-key.js 98.97% <0.00%> (-0.12%) ⬇️
lib/rules/no-arrow-function-lifecycle.js 98.36% <0.00%> (-0.08%) ⬇️
index.js 100.00% <0.00%> (ø)
lib/rules/hook-use-state.js 100.00% <0.00%> (ø)
lib/util/isCreateElement.js 100.00% <0.00%> (ø)
lib/rules/no-unknown-property.js 98.82% <0.00%> (ø)
lib/rules/static-property-placement.js 100.00% <0.00%> (ø)
lib/rules/sort-default-props.js

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ljharb
Copy link
Member

ljharb commented Sep 10, 2022

um, what? The new system is ESM? Why can’t it be CJS?

@jjangga0214
Copy link
Contributor Author

jjangga0214 commented Sep 11, 2022

@ljharb Your point is right.
Though eslint recommends ESM for the new config system, it supports CJS as well.

So, in conclusion,

if ( require()d ){
  the user is either using legacy or new config system. 
} else if( import()ed ) {
  the user is using new config system
}

For supporting the new config system in CJS, I patched .mjs to .js and introduced eslint-plugin-react/new in the new commit.

@ljharb
Copy link
Member

ljharb commented Sep 11, 2022

Native ESM doesn't have any benefits over CJS (or transpiled ESM), and lacks the majority of the relevant tooling capabilities, so I'd strongly prefer to avoid using it entirely - let's keep everything in CJS.

What is the benefit of the new config system? Why does an eslint user care what config system this plugin is using?

@jjangga0214
Copy link
Contributor Author

jjangga0214 commented Sep 11, 2022

What is the benefit of the new config system?

You may want to read these if you haven't.

Personally, I felt the resolution of this issue eslint/eslint#3458 is an impressive benefit.

Why does an eslint user care what config system this plugin is using?

Because the current config is not compatible with the new system.
Users who want to migrate to the new system need this support.

@jjangga0214
Copy link
Contributor Author

jjangga0214 commented Sep 11, 2022

스크린샷 2022-09-11 오후 7 52 54

I added a new dependency named globals.
This requires a higher engines limitation.
How would we do?

  1. Increase engines.
  2. Or embed a part of globals. This would not be a hard task.

@ljharb
Copy link
Member

ljharb commented Sep 11, 2022

That issue benefits shared configs, not plugins.

configs/recommended.js Outdated Show resolved Hide resolved
@jjangga0214
Copy link
Contributor Author

Okay, I rewrote the config without files and globals, and README as well.
Would you please check them out?

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide 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/mtsx/mjsx extensions (native ESM should always be .mjs, not .js, so it's critical to allow that), converted the CJS files you added to ESM, and decided to put the configs under a top-level configs directory instead, and refactored index.js to pull from those.

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.

README.md Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants