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

flat config implementation looking for feedback #707

Closed
wants to merge 7 commits into from

Conversation

spence-s
Copy link
Contributor

@spence-s spence-s commented Mar 3, 2023

Here is a flat config implementation that somewhat implements the proposals I made in here

  • has a file/function that creates a single config that can be used by eslint directly and lints in 1 eslint pass
  • POC for merging in current configs to the flat config

Basically I've taken this pretty far without hearing back whether its a reasonable approach or not.

Personally, I don't think we should be trying to merge all the old configs and cli options together with this but I went that direction to see and show how it could be done, but it could be made simpler if the "flat" option respected ONLY the flat config.

There are some challenges and changes we need to make as it is not 100% compatible with the way xo works now since we lint in 1 pass. We have a little less info on a file by file basis and there are many optimizations we can make based on the direction we want to go with it.

Happy to answer any questions about this and about other potential directions we could go since I've seen the capabilities and options we have with the flat config.

Once we decide on a direction for something like this, there needs to be a lot more tests added, and some behavior tweaked so this will not be mergable until that time.

@@ -31,6 +32,7 @@ const cli = meow(`
--stdin Validate/fix code from stdin
--stdin-filename Specify a filename for the --stdin option
--print-config Print the effective ESLint config for the given file
--flat Use the experimental flat config.
Copy link
Member

Choose a reason for hiding this comment

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

If already reliable, I think it's best to switch to the flat config rather than add another thing to maintain. There's no advantage for the user to run this experiment I think.

As a tester I'd probably prefer trying a flat-only version published from this branch, rather than something that will eventually require further refactoring and further testing.

Note: I'm not a maintainer here so this is just an opinion

Copy link
Contributor Author

@spence-s spence-s Mar 3, 2023

Choose a reason for hiding this comment

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

@fregante the problem right now is that the flat config is not currently reliable (on eslints side) the api will change, and I had to implement a couple of hacky work arounds until eslint gets it stable.

The benefit to the user right now is that they can use third party plugins and override ours without resolution problems. So there would be benefits to using it right now if you want to add or remove plugins and third party configs or support something like mdx or whatever else eslint can support, which is impossible (or difficult and bug prone) with xo today.

its also worth noting that the way I designed this is practically an entire new code base as it works totally different than xo today. It hands off a lot more work to eslint and just does some work to intelligently create the flat config.

@spence-s
Copy link
Contributor Author

spence-s commented Mar 3, 2023

To make reviewing this a little more digestible for people to weigh in on, here are the questions we need to answer:

  1. Do we want to pursue the single config and 1 pass linting so that users can use the xo config with eslint directly? (obviously the most important question, We could potentially keep the internals the same and switch to flat config still although I feel like that is not the best choice personally).

The rest of the question assume that we do want to export the eslint compatible config and use 1 pass linting.

  1. What do we do with the current cli options (in this example I attempted to merge them in to the flat config)
  2. What do we do with other configs that act like cli options (ie... package.json config, here I attempt to merge in options from package.json config)
  3. Assuming we support cli options or other config options still, we need to be clear to the users on how those options get merged into the flat config. This is something that could yield slightly different behaviors based on our approach.
  4. What is the best way to support type aware typescript linting for users.

If we don't want to support the 1 pass linting then we need to answer how we can support it because it will be the future of eslint AFAICT, but it may be a good while before its THE way for eslint.

@sindresorhus
Copy link
Member

I like how you made the flat codebase separate. That means it's easy to switch to it and remove the old one when the time comes.

@sindresorhus
Copy link
Member

Personally, I don't think we should be trying to merge all the old configs and cli options together with this but I went that direction to see and show how it could be done, but it could be made simpler if the "flat" option respected ONLY the flat config.

I agree. The flat option should only respect flat config.

@sindresorhus
Copy link
Member

Thanks for working on this. And sorry for the late reply. I just had a lot of things to reply to lately.

@spence-s
Copy link
Contributor Author

I like how you made the flat codebase separate. That means it's easy to switch to it and remove the old one when the time comes.

Great!

I agree. The flat option should only respect flat config.

Awesome, this choice will make the v1 --flat be much more scoped and easier to test. We can always expand back later if the community really misses some simple pkg json configurations or something.

Thanks for working on this. And sorry for the late reply. I just had a lot of things to reply to lately.

No worries! I know how it is, I am working on this in little bursts too when I have time, so it might be a while until a real PR is ready.


per comments in #702

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.

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

We can definitely fully support type aware linting as it is now. However, to support this, I am not going to use the custom TS lookup we have currently in xo, I am using TS directly.

  • The downside of this is that it's a much slower lookup and is synchronous.
  • The upside is we don't have to worry about breaking changes in tsconfig (ie...like the recently release v5 extends field which supports an array of extends and will break the current custom lookup logic we have in xo).
  • setting parserOptions.project will opt the user out of all slow lookups. I think this is a great middleground between usability and performance.

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

Awesome! I moved some of this work to a TS repo that I haven't pushed to github yet because I wasn't sure if my approach here was going to be well received and I really like working in TS for this stuff, once I get it to a point where a PR is ready to be made, we can discuss how we want combine the efforts here!

Ultimately, want 1:1 lint results with the new --flat option, just new ways to configure things and add plugins etc.

@spence-s
Copy link
Contributor Author

For anyone interested, I ceased this effort after a good chunk of development because initial tests were unacceptably slow.

The project is not ready to be used generally and works with some configurations but is still very buggy, but after seeing the performance degrade by so much with the flat confid I decided I would wait on this effort until eslint has made this config style more mature and deprecates the old config style. At that point they will be forced to address performance concerns and show best practices.

It may be the slowness comes from trying to put too much into 1 config that can lint both ts and js, however, theoretically it should be possible and seemed like the "cleanest" solution to me.

The full code can be found here: https://github.com/spence-s/flat-xo

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

3 participants