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 #1245

Closed

Conversation

jjangga0214
Copy link

Supporting eslint's new config system of eslint.

The legacy config system always has require()d plugins and sharable configs, while the new system is available in both ESM and CJS.

Therefore conditional export is great to keep compatibility while introducing the new config.

plugin: protocol(e.g. plugin:jest/recommended) is not valid any more. In the new config system, a bi-directional dependency between a plugin object and a preset config object is detached. From now on, only a config object should depend on a plugin object, not vice-versa. Thus, the new plugin (a new entry point index.js) doesn't have configs property, following the spec, though having the property does not mean incompatibility.

Users should have a way to import shareable configs independently.
Hence eslint-plugin-jest/all, eslint-plugin-jest/recommended, eslint-plugin-jest/style are introduced as new entrypoints.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

super exciting, thanks!

"main": "lib/legacy.js",
"exports": {
".": {
"import": "./lib/index.js",
Copy link
Member

Choose a reason for hiding this comment

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

hmm, I don't think we produce ESM?

Copy link
Author

Choose a reason for hiding this comment

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

exports can benefit CJS projects, like eslint-plugin-jest.

There's a difference in module resolution between the legacy and new config system.

In the legacy config system, as we know,

  • A shareable config or plugin is not loaded(require or import) in eslintrc.js.
  • Instead, a user just specifies the name as a string (e.g. plugin:jest/recommended or plugin: [ 'jest' ]), and eslint internally require() (not import()) them.

In the new config system,

  • A shareable config or plugin is explicitly loaded by a user, directly or transitively, following the native module resolution by nodejs. eslint does not load by itself anymore.
  • A shareable config or plugin can be required or imported, as eslint support both CJS and ESM for eslint.config.js.

Thus,

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

Therefore, by this PR,

  • A user who uses the legacy config system can let eslint to do require('eslint-plugin-jest'). This is not breaking change.
  • A user who uses the new config system in ESM can import jest from 'eslint-plugin-jest'. This feels natural. This can be possible by the conditional export.
  • A user who uses the new config system in CJS can require( 'eslint-plugin-jest/new').

@SimenB SimenB requested a review from G-Rath September 14, 2022 13:39
@G-Rath
Copy link
Collaborator

G-Rath commented Sep 16, 2022

tbh I'm kind of tempted to ignore the new config format until v9 of ESLint is released, from a documentation perspective - I'm really looking forward to it and am happy with doing the minimum we need to let people start using it now, but it feels like it could be taxing on our brains to deal with ensuring we have clean documentation for both config formats everywhere (we could still have something saying we do support the new config format though).

What do people think?

@mrazauskas
Copy link
Contributor

mrazauskas commented Sep 17, 2022

What do people think?

Adding support for the new config format is very good idea, but the amount of documentation is currently overwhelming.

How do other plugins deal with this? Perhaps it would be enough mentioning: "eslint-plugin-jest supports the new config format since vX.X. Usage example:" and code block?

@jjangga0214
Copy link
Author

I think we don't have to change docs/rules/*.md.
Because the rule usages are compatible with the new config system.
The only thing to change is readme.
And this is just done in this PR.

@G-Rath
Copy link
Collaborator

G-Rath commented Sep 21, 2022

@jjangga0214 my understanding is that technically the config for rules is slightly changed, but it is actually the Readme change that I'm not happy with currently - I think we should just have a short sentence saying we're compatible with the new config system with a link to the docs, and leave it as that for now.

Then we can switch over when ESLint v9 is released.

@jjangga0214
Copy link
Author

@G-Rath
It makes sense logically.
However, psychologically, a person can feel assured by our a little bit detailed docs.
Eslint's docs only describe how the new system works, not how a specific shareable config should be configured.
Of course, people can indeed infer how it should be configured after reading eslint's docs, but having assurance is a different matter.
This feeling can trigger people to migrate more confidently, being beneficial at the beginning of the new specification.
So we can make our docs concise later, when people get used to the new config system.

@G-Rath
Copy link
Collaborator

G-Rath commented Sep 26, 2022

@jjangga0214 I understand that, but that documentation should ideally be consistent with the surrounding content otherwise it can be jaring and confusing, which can undermine that feeling of assurance.

I'm generally happy to help with documentation but I'm pretty busy right now, still have not had a chance to checkout the new config system myself, and in the meantime (from what I understand) without this PR people can't use this plugin at all with the new config system; so I think let's go with the simpler form here to get this landed, and then we can bikeshed about the documentation in another dedicated PR :)

(Also ESLint v8 is about two weeks shy of being a year old, so we might only be waiting a month or two before v9 comes out anyway)

@jjangga0214
Copy link
Author

@G-Rath I allowed editing by maintainers.
You can remove the new description and add some simpler sentences you want.

@G-Rath G-Rath marked this pull request as draft November 19, 2022 23:35
@acommodari
Copy link

As someone who's also trying to migrate to the new config format and uses this plugin it would be great to get support on this.

@morganney
Copy link

morganney commented Jul 18, 2023

Either conditional exports or create a new package name since plugins no longer need to be prefixed with eslint-plugin.

Either way, would be nice to see this effort completed. Currently, I'm modifying it myself in eslint.config.js like

import js from '@eslint/js'
import globals from 'globals'
import jest from 'eslint-plugin-jest'

/**
 * Can't extend jest's configuration due to the
 * incompatibility between the classic config,
 * and the new flat config. Change it here, for now.
 * @see https://github.com/eslint/eslint/issues/17355
 */
jest.configs.recommended.plugins = { jest }
jest.configs.recommended.languageOptions = {
  globals: {
    ...globals.jest
  }
}
delete jest.configs.recommended.env

export default [
  js.configs.recommended,
  jest.configs.recommended,
  {
    languageOptions: {
      ecmaVersion: 2023,
      sourceType: 'module',
      globals: {
        ...globals.node,
        ...globals.jest
      }
    },
    files: ['**/*.js'],
    plugins: {
      jest
    },
    rules: {
      'no-console': 'error'
    }
  }
]

@G-Rath
Copy link
Collaborator

G-Rath commented Jul 26, 2023

Closing in favor of #1408

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

Successfully merging this pull request may close these issues.

None yet

6 participants