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 9 #705

Open
1 task done
paularmstrong opened this issue Apr 19, 2024 · 7 comments
Open
1 task done

Support ESLint 9 #705

paularmstrong opened this issue Apr 19, 2024 · 7 comments
Labels
🚀 feature New feature or request 🚑 help wanted Extra attention is needed 🧩 plugins Official plugins

Comments

@paularmstrong
Copy link
Owner

Checklist

  • This work is already approved through discussions.

Problem

@onerepo/plugin-eslint does not work with ESLint ^9

Suggested solution

May need special handling between the two versions or else a new major version that only supports ESLint 9

@paularmstrong paularmstrong added 🚀 feature New feature or request 🚑 help wanted Extra attention is needed 🧩 plugins Official plugins labels Apr 19, 2024
@Thisisjuke
Copy link

Hi !
I am developing an intern solution (CLI) that should generate a monorepo based on our company rules and libs. Right now, we are doing so using NX.dev. We are looking and open for any solution based on easy generating as we have some "complex" workflows / plugins and we were testing a bit of onerepo.

Eslint FlatConfig full support is the task that is giving us the biggest headache, and we can't go without it because we will have to do it when the deprecated eslintrc format will be deleted : https://eslint.org/docs/latest/use/configure/configuration-files-deprecated

What NX is doing right now is sharing a CLI command to convert eslintrc config to FlatConfig formatted config files:

Might be a starting point / inspiration for your plugin ?

Everything is based on the :

const { FlatCompat } = require('@eslint/eslintrc')

But there are some dirty workaround if in the future @onerepo/plugin-eslint will include other eslint plugin as dependencies, for example a "Full TS - Onerepo eslint plugin".
(seems like this not the case and not the way you will go: https://github.com/paularmstrong/onerepo/blob/main/plugins/eslint/package.json).

Example of an Ugly workaround we had do to, to prevent the fact that both antfu and our custom eslint lint were exporting (extending in fact) the react plugin:
Error: key "plugins": Cannot redefine plugin "react".


const { FlatCompat } = require('@eslint/eslintrc')
const js = require('@eslint/js')
const ts = require('@typescript-eslint/eslint-plugin')
const hooks = require('eslint-plugin-react-hooks')
const next = require('@next/eslint-plugin-next')
const nx = require('@nx/react')
const a11y = require('eslint-plugin-jsx-a11y')

const compat = new FlatCompat({
baseDirectory: __dirname,
recommendedConfig: js.configs.recommended,
})

module.exports = [
...compat.extends('plugin:@nx/react-typescript', 'next', 'next/core-web-vitals').map((config) => {
return ({
...config,
plugins: {
'@typescript-eslint': ts,
'react-hooks': hooks,
'@nx/react-typescript': nx,
'@next/next': next,
'jsx-a11y': a11y,
},
files: ['/*.js', '/.jsx', '**/.ts', '**/*.tsx'],
})
}),
]

The fact that there are no clear information across the web about FlatConfig may become the highest wall atm 😞. In the NX context, the fact that eslint FlatConfig CLI command is in the "tips'n'tricks" section of their documentation is not really reassuring either...

Also, some HUGE chunks of mandatory resources like globals are missing clear documentation and the information found online is not the freshest.

On our side, the full FlatConfig support is coming form the fact that we want to use tools like https://github.com/antfu/eslint-config that are already fully supporting the eslint FlatConfig spec.

Don't know if its really helpful but I hope you will find what you need inside the NX migration script or :

const { FlatCompat } = require('@eslint/eslintrc')

@TzviPM
Copy link
Contributor

TzviPM commented Dec 9, 2024

A few comments on this feature:

  1. The eslint CLI no longer supports filtering by extensions. Instead, the --ext argument can only be adjusted from the config file. Similarly, .eslintignore is deprecated in favor of an ignores key.

The internal base config would also need to be represented as a flat config. My suggestion would be something like tseslint.config which exposes flat configs that can be spread as well as a helper function to handle nested flat configs. We could even re-export it: https://typescript-eslint.io/packages/typescript-eslint#config

@paularmstrong
Copy link
Owner Author

@TzviPM As much as possible, oneRepo should avoid creating custom configurations for other tools. I don't see any reason to do anything special with regards to how the ESLint configurations are constructed. The current plugin does not do any referencing or checks on the configuration file.

The only thing that may or may not be necessary is to parse the new eslint flat config in order to read the ignored files (done here, with test) in order to avoid sending already ignored files to the linter. That being said, I'd be fine with recommending using the .gitignore solution and encourage using /* eslint-disable */ in files if possible.

It's been too long since I've looked at the problems that ESLint 9 gave me, but I think most of them revolved around third-party/custom rules and plugins, which made it a non-starter for even testing within the oneRepo git repository itself.

@TzviPM
Copy link
Contributor

TzviPM commented Dec 9, 2024

@paularmstrong yes, sorry. To clarify, I was referring to the internal package that’s used by the onerepo eslint config itself. That package will need to export rules as an array and then our eslint.config.js file will need to spread it into its own config. It seems most of the 3rd party plugin issues are resolved. One of the big ones was the import plugin that was blocking many downstream plugins that relied on it

@TzviPM
Copy link
Contributor

TzviPM commented Dec 9, 2024

If you’re open to a version that drops eslint 8 support, I can put up a PR. Otherwise, we need to figure out how we want to allow backwards compatibility. I think tools like nextjs are setting an environment variable ESLINT_USE_FLAT_CONFIG=false based on some heuristic. We could provide a configuration option like ‘useFlatConfig’ that could be set to false whereby we could set the corresponding environment variable. Thoughts?

@paularmstrong
Copy link
Owner Author

I'm a little hesitant to fully drop eslint 8 support, as I know of a lot of teams (myself included) that are having a hard time justifying putting in the work for the update.

I prefer to avoid environment variables on configurations as much as possible, and your alternative suggestion for a config option on the plugin itself would make more sense anyway, given that you want to ensure it's set specifically for the repo itself.

I can think of two alternatives, but still prefer your suggestion. Adding those here just in case:

  1. Determine the installed eslint version by reading the version from the root package.json
  2. Whether the root workspace has one of eslint.config.* (v9) vs .eslintrc* (v8) files.

Neither of these alternatives is as performant or as obviously strict as an explicit configuration option.

One of the big ones was the import plugin that was blocking many downstream plugins that relied on it

Ah yes! Thanks for mentioning… I think this was my biggest blocker as well. Good to know it's been updated

@TzviPM
Copy link
Contributor

TzviPM commented Dec 14, 2024

The environment variable is part of eslint 9 itself. We can keep eslint as a peer dependency with version "^8 || ^9" and then keep our own understanding of which eslint version and which config file format. For the CLI commands, I believe the --ext flag is not supported in eslint 9 even with ESLINT_USE_FLAT_CONFIG=false, but setting extensions in the config is supported in eslint 8. In that case, we could safely deprecate it in a major version of onerepo with a clear migration path of using the config file.

We can provide out own useFlatConfig option in the eslint plugin that will set the environment variable when calling eslint.
In theory, option 1 is not necessary, because if they are using eslint 8, then when we shell out to the eslint cli, it will "just work". Option 2 is also not necessary, because they can just set the config option. However, we could try to infer the useFlatConfig option based on the presence of the root config files if it's not explicitly passed. thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚀 feature New feature or request 🚑 help wanted Extra attention is needed 🧩 plugins Official plugins
Projects
None yet
Development

No branches or pull requests

3 participants