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

Support ESLint new config system #1886

Merged
merged 35 commits into from
Dec 20, 2023

Conversation

jjangga0214
Copy link
Contributor

@jjangga0214 jjangga0214 commented Sep 10, 2022

Supporting eslint's new config system of eslint.

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

Thus conditional export is great to keep compatibility.

Fixes #1885
Fixes #2220

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

jjangga0214 commented Sep 10, 2022

@sindresorhus

If I try to create a "drop-in" replacement for the old config system, recommended.js would be like this.

import unicorn from '../index.mjs'
import globals from 'globals'

export default {
	languageOptions: {
		globals: {
			...globals.node,
			...globals.es2021
		}
	},
	plugins: {
		unicorn,
	},
	rules: { /* ... */ }
}

However, in the new config system, there is actually no such "root" config.
Everything is kind of overrides, which was implemented comparatively recently on the old config system.
So the main intention of design is chaining.

languageOptions is a root-like options. So I am not sure if it'd be a good convention for a sharable config to provide the default languageOptions in the new config system.

If we include the default languageOptions, then the end-user might feel convenient when they use unicorn as a drop-in config, but otherwise have to manually override languageOptions.

Which one do you prefer?

@jjangga0214
Copy link
Contributor Author

@sindresorhus And I also want to ask how you think about including files options.

export default {
	files: ['**/*.{js,cjs,mjs,jsx,ts,tsx}'],
	plugins: {
		unicorn,
	},
	rules: { /* ... */ }
}

@sindresorhus
Copy link
Owner

languageOptions is a root-like options. So I am not sure if it'd be a good convention for a sharable config to provide the default languageOptions in the new config system.

I would prefer to keep including them. We can reevaluate it when the new config system is more mature and has established conventions.

@sindresorhus
Copy link
Owner

And I also want to ask how you think about including files options.

If that's not the default in the new system, it makes sense to include it here for convenience.

@sindresorhus
Copy link
Owner

You need to fix the lint issues.

@jjangga0214
Copy link
Contributor Author

jjangga0214 commented Sep 11, 2022

I would prefer to keep including them. We can reevaluate it when the new config system is more mature and has established conventions.

✅ Done.

If that's not the default in the new system, it makes sense to include it here for convenience.

스크린샷 2022-09-11 오후 8 16 56

✅ It's not default and I've configured it.

You need to fix the lint issues.

✅ Right. Forgot to do so. Fixed :)

@sindresorhus
Copy link
Owner

Linting is still failing

@jjangga0214
Copy link
Contributor Author

jjangga0214 commented Sep 11, 2022

@sindresorhus Well.. right, fixed again.

@jjangga0214
Copy link
Contributor Author

I wrote a guide for the new config system and usage in readme.
Please check it out :)

readme.md Outdated Show resolved Hide resolved
sindresorhus and others added 2 commits September 20, 2022 15:49
Co-authored-by: Ari Perkkiö <ari.perkkio@gmail.com>
@sindresorhus
Copy link
Owner

sindresorhus commented Sep 20, 2022

I think the docs are a bit too verbose. It's not our job to document how the ESLint config system works. We should just show a small example and then link to ESLint docs for more.

@jjangga0214
Copy link
Contributor Author

jjangga0214 commented Sep 21, 2022

It's not our job to document how the ESLint config system works.

Yes, I agree that's 100% right logically.
However, psychologically, a person can feel assured by our a little bit verbose docs.
Eslint's docs only describe how the new system works, not how the specific shareable config(unicorn's) should be configured.
Of course, people can indeed infer how unicorn 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.

@fisker
Copy link
Collaborator

fisker commented Dec 20, 2023

I decided to remain CommonJS support in this PR, at least before eslint/eslint#17863 gets solved, people still need the config file in CommonJS.

@fisker
Copy link
Collaborator

fisker commented Dec 20, 2023

Questions:

  • Do we want the entry point to be eslint-plugin-unicorn/configs/recommended or eslint-plugin-unicorn/recommended (@jjangga0214 proposaled original)
  • Do we still want to keep the equivalence of {env: {es2024: true}} (legacy), which adds globals and ecmaVersion(already take care). To make it work as before I use @eslint/eslintrc to get the globals
  • Anything else not addressed?

@sindresorhus

@sindresorhus
Copy link
Owner

Do we want the entry point to be eslint-plugin-unicorn/configs/recommended or eslint-plugin-unicorn/recommended (@jjangga0214 proposaled original)

I think we should stick to what ESLint recommends, just unicorn.configs.recommended.

https://eslint.org/docs/latest/use/configure/configuration-files-new#using-configurations-included-in-plugins

@sindresorhus
Copy link
Owner

Do we still want to keep the equivalence of {env: {es2024: true}} (legacy), which adds globals and ecmaVersion(already take care). To make it work as before I use @eslint/eslintrc to get the globals

No, I don't think that makes sense. We can rather document it and also include it in the recommended config.

@sindresorhus
Copy link
Owner

Anything else not addressed?

Just make sure it follows the recommendations in https://eslint.org/docs/latest/extend/plugin-migration-flat-config#backwards-compatibility

@fisker
Copy link
Collaborator

fisker commented Dec 20, 2023

just unicorn.configs.recommended.

We can't, we already have unicorn.configs.recommended as legacy config. So unicorn.configs['flat/recommended'] for now?

@sindresorhus
Copy link
Owner

We can't, we already have unicorn.configs.recommended as legacy config. So unicorn.configs['flat/recommended'] for now?

Yes. Let's do it like that and we can switch it around later on when we fully move to flat config.

@jjangga0214
Copy link
Contributor Author

jjangga0214 commented Dec 20, 2023

This would be better then, IMHO.

import unicorn from 'eslint-plugin-unicorn/flat'

export default [
        unicorn.configs.recommended,
]

@fisker
Copy link
Collaborator

fisker commented Dec 20, 2023

This would be better then, IMHO.

import unicorn from 'eslint-plugin-unicorn/flat'

export default [
        unicorn.configs.recommended,
]

We can switch back to unicorn.configs.recommended in the future.

@jjangga0214
Copy link
Contributor Author

jjangga0214 commented Dec 20, 2023

@fisker I know. But I mean /flat entry point until then. I guess maybe you missed reading it?

@fisker
Copy link
Collaborator

fisker commented Dec 20, 2023

If we don't add /flat entry, we don't need define exports.

If we add /flat.js file, people need use

import unicorn from 'eslint-plugin-unicorn/flat.js'

and

const unicorn = require('eslint-plugin-unicorn/flat')
// OR
const unicorn = require('eslint-plugin-unicorn/flat.js')

I think unicorn.configs['flat/recommended'] is fine.

@fisker fisker changed the title feat: support new config system Support ESLint new config system Dec 20, 2023
@fisker
Copy link
Collaborator

fisker commented Dec 20, 2023

I think it's ready for review.

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.

Add support for flat config (eslint.config.js) Export new eslint config
4 participants