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

Move to flat config #702

Open
Tracked by #18093
fregante opened this issue Jan 4, 2023 · 14 comments
Open
Tracked by #18093

Move to flat config #702

fregante opened this issue Jan 4, 2023 · 14 comments

Comments

@fregante
Copy link
Member

fregante commented Jan 4, 2023

https://eslint.org/blog/2022/08/new-config-system-part-2/

The change has been out for a while and it might be a good time to start exporting it. I believe this will solve a lot of config-related issues:

Some notes around this in #428 (comment)

Upvote & Fund

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@chrisspiegl
Copy link

This seems to tackle a lot of things I have recently run into. Looking forward to the integration.

@spence-s
Copy link
Contributor

spence-s commented Feb 20, 2023

I wanted to start this effort. But I think first, we need to think about what it means for xo as a whole. Ultimately, the original goal for xo (a shareable config that includes other configs and plugins I believe) is now complete. Eslint has also simplified their configuration a lot with the new flat config, and most of the original goals of xo have been solved with eslint.

What does this mean for xo then?

This means that we could potentially make xo nothing more than an eslint config and drop our cli/editor extensions and still hit 90% of xo use cases. Sherriff basically takes this approach here.

However, there are still a few features which justify continuing to maintain the xo cli. They are mainly the output formatter, the caching logic, and maybe a few other cases, I haven't discovered. I think this good enough reasons to keep maintaining a cli and editor plugin ecosystem.

Proposal:

Rewrite xo to:

  1. export a single function which accepts an xo flat config (see 3 below) and returns an eslint flat config that contains most of the logic in xo now.
  2. js and ts can be lint in a single linting pass whether it is ran with xo directly or with eslint the results will be the same. Some xo config options which don't affect lint results will be ignored when using the config function directly.
    • EDIT: May be some complications with our auto tsconfig generation for type aware ts linting which may make it difficult or impossible to achieve the same results with eslint and direct xo runs for ts, but I hope in the re-write we can smooth this behavior because its both a big source of complexity in xo and a source of a lot of bugs and confusion for xo users, but its a huge convenience tool.
  3. the cli only needs to be simplified to the following.
    • Accept a single xo.config.js as a configuration mimicking the new eslint practice.
    • The xo.config.js file will export an array of xo configs
    • xo configs can largely stay as they are now and the changes will reflect the eslint config changes
      • overrides will be removed
      • extends will be removes
      • plugins will take an object now
      • There are probably other adjustments that will be found that need to be made to the configs/cli but they should be relatively minor.

Why a full rewrite?

Maybe not full, I think we can mostly keep the same cli.js file. xo, as is, is designed to run eslint multiple times for each xo configuration file group. However, this can be natively accomplished with eslint now with a single pass with much more efficiency. It will be much cleaner to rewrite the logic in our index.js and options-manager.js files from the ground up than to modify them to support the new stuff.

Backwards compatibility

We have a lot of options to maintain backwards compat behind a flag or option for a while if we want. Or we can simply rip the bandaid off in a version bump.

TL;DR: xo should start accepting its own "flat config" and be re-written for better interop and efficiency with eslint while keeping most (or all) of its current functionality and at the same time, be greatly simplified internally.

@fregante
Copy link
Member Author

fregante commented Feb 20, 2023

Love the idea, some of that is also covered/discussed in:

Such a change would also get the funding from:

@spence-s
Copy link
Contributor

spence-s commented Feb 23, 2023

I've got a good start on this, I am going on vacation starting saturday for a couple of weeks, but I think I can get a draft PR for this before then, if not it should be in shortly when I get back.

Some more thoughts we can discuss and how my initial implementation looks:

  1. I am going to put the flat config stuff behind a --flat flag (for the cli) and not change anything about normal xo usage, it will be totally opt in. The API will stay exactly the same (for editor integrations) but and editor integrations can expose the flat config on their own.
  2. flat config will only read from xo.config.js and will use the standard cli options we have now as much as possible.
  3. We can definitely export a function which creates an eslint config for easy interop for those who want to use eslint directly.
  4. As implied by above we can lint js or ts in 1 pass and lint exactly the same. It appears to be more performant at first glance, but that needs a lot closer inspection, may be memory tradeoffs and other things.

Some potential downsides/pitfalls:

  1. It's really too early to lean on flat config entirely for xo behavior. The API is still changing, and there are still bugs in the new system on ESLints side. So maintaining xo and xo --flat will require modifying code in 2 places when a change is made. So this is a bit of a maintenance burden. However, I think the tradeoff is worth it for the code simplification we'll eventually get as we move completely over to the flat implementation. I think it's worth bringing up to hear opinions on this tho. We could extract some common functionality better to make this a moot point.
  2. Since we are taking a new direction with how xo behaves internally for the flat config, most of our tests can not be applied to it, so new tests will need to be written from scratch. This kind of sucks since we have a great test suite. But since we are doing a lot less work with the new code, maybe it won't be too painful to migrate a lot over.

New possibilities/features for xo:

  1. As I have been thinking about ways to simplify xo and keep it modern and usable, I have been thinking about our handling of tsconfigs for type aware linting on any file. This is a cool feature of xo where you don't need one at all to get linting, we do all the work and generate a temp one for you if we can't find yours. However, it's a lot of lookup work on our end to find and create this file and has been a big source of bugs for xo.

    I was thinking with the flat config, we would change this behavior slightly. I would propose that we separate the type aware rules from the non-type aware rules, and only turn on the type aware rules if a user supplies a tsconfig. We could also expose a tsconfig option for convenience as its more clear than "parserOptions.project" field.

    If a user passes this field, then we turn on the type aware rules, if they don't then we turn off the type aware rules.

    Users still get linting on any ts file with no configuration, and we can avoid expensive and bug prone tsconfig lookups on every run.

    An added tradeoff of this approach is that users can now have the option to not use type aware linting at all (which they currently do not really have wo manually figuring out which rules to turn off). This can be a huge performance boost for linting large ts projects. The downside is the added configuration and not being type aware by default.

  2. One thing that is tough about working with xo right now as a maintainer is we have one options object that gets passed around and changed by a bunch of different functions and its mentally really hard to keep up with. I am adding ts via jsdoc to the new flat internal code. Some of this typings gets passed to the legacy stuff as well and will help future maintainers (myself 😂 ) be more confident in their code. I think it would be good to make ts checks as part of ci validation as well.

@sindresorhus
Copy link
Member

I was thinking with the flat config, we would change this behavior slightly. I would propose that we separate the type aware rules from the non-type aware rules, and only turn on the type aware rules if a user supplies a tsconfig. We could also expose a tsconfig option for convenience as its more clear than "parserOptions.project" field.

The point of TypeScript is to have types. Same applies to linting. I'm not very interested in supporting use-cases for not using the type aware rules.

Users still get linting on any ts file with no configuration, and we can avoid expensive and bug prone tsconfig lookups on every run.

ESLint may support types built-in at some point: eslint/eslint#16557

One thing that is tough about working with xo right now as a maintainer is we have one options object that gets passed around and changed by a bunch of different functions and its mentally really hard to keep up with. I am adding ts via jsdoc to the new flat internal code. Some of this typings gets passed to the legacy stuff as well and will help future maintainers (myself 😂 ) be more confident in their code. I think it would be good to make ts checks as part of ci validation as well.

I would be fine with using TypeScript for XO. That would be preferable over lots of JSDoc comments.

@yangmingshan
Copy link

I would love to see xo become to a simple eslint config, at least provide one.

I've been using xo as a eslint config set for years actually 😂

@spence-s
Copy link
Contributor

spence-s commented Jul 19, 2023

Just an update here... I have worked on this a bit and have a version which works ok. It doesn't quite cover all xo options correctly yet, but it does cache ESLint instance and lint in 1 pass and has similar lint results (not quite 1 to 1).

Unfortunately the initial results show that a large and complex ESLint flat config is quite a bit slower than what xo does now, even though we can cache the eslint instance, which is surprising.

this implies that either:

  1. I implemented things in a way that is very unoptimized for ESLint and it can be drastically improved by re-doing the work I have already done.
  2. ESLint Flat Config internal parsing is just much slower.

It is likely a combination of both factors.

Since I discovered that its a big downgrade in performance I haven't worked on it anymore, and I've lost my motivation for this effort...

I think what I did was a good experiment, but now I think we maybe should take the path of least resistance with upgrade to flat config and use the current code base and just hook in the eslint compatibility libs and continue to use xo as it is. --- Not sure how the performance will look then.

https://github.com/spence-s/flat-xo for anyone who would like to review or try out the code.

@sindresorhus
Copy link
Member

This is put on a break until ESLint improves flat config performance: #707 (comment)

@fregante
Copy link
Member Author

fregante commented Oct 1, 2023

Here are some other projects tackling this issue, for reference:

@sindresorhus sindresorhus pinned this issue Jan 16, 2024
@sindresorhus sindresorhus changed the title Use flat config in eslint-config-xo Move to flat config Mar 8, 2024
@polar-sh polar-sh bot added the Fund label Mar 8, 2024
@sindresorhus
Copy link
Member

This issue is now funded.

@spence-s
Copy link
Contributor

Took a look at this repo again over the weekend. https://github.com/spence-s/flat-xo
The performance has definitely improved and things definitely seem to be more stable than the last time I looked (almost a year ago) and I feel like its ok to keep moving forward with this effort for me...

However, there are still some things to note.

It seems these plugins do not yet support flat configs. (see eslint/eslint#18093)

  • eslint-plugin-comments
  • eslint-plugin-import

Also, there is a ton of work to do for testing, many of xo's current tests are about parsing the old config type so those can't be ported over. Also the cli flags are changing so tests around those can't be ported either.

Anyway - I am going to continue work in the flat-xo repo, I welcome anyone who wants to contribute there, and when I get some better tests and stability, we can figure out the best way to get the code in the new repo under xo namespace/cli and all that good stuff.

If anyone wants to try to beat me to this I would encourage them to try, I think this thing is still a good while away from stability.

@voxpelli
Copy link
Contributor

@jlarmstrongiv
Copy link

@fregante
Copy link
Member Author

fregante commented Apr 6, 2024

Ideally yes, but I assume that support will follow later.

Other than eslint-config-xo, they only exist as standalone repos to be easily selectable in the old eslintrc-style config. The Flat Config is dynamic so one package can be configurable.

IMO eslint-config-xo can/should be converted first, and then this executable can follow, but I don't have full insight into what xo does.

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

No branches or pull requests

7 participants