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

Config File Simplification #9

Merged
merged 78 commits into from
Jul 10, 2020
Merged

Config File Simplification #9

merged 78 commits into from
Jul 10, 2020

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Jan 22, 2019

Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

Nice work! This looks very promising!

Identified a few minor typos (the first one is unfortunately breaking the formatting in the PR diff view). Wasn't too sure about one of them, so please check before applying suggestions. Thanks!

designs/2019-config-simplification/README.md Outdated Show resolved Hide resolved
designs/2019-config-simplification/README.md Outdated Show resolved Hide resolved
designs/2019-config-simplification/README.md Outdated Show resolved Hide resolved
designs/2019-config-simplification/README.md Outdated Show resolved Hide resolved
Copy link
Member

@gyandeeps gyandeeps left a comment

Choose a reason for hiding this comment

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

Gonna review more once all the red text is fixed.... it was little hard to read. 😸

designs/2019-config-simplification/README.md Outdated Show resolved Hide resolved
designs/2019-config-simplification/README.md Outdated Show resolved Hide resolved
Copy link
Member

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

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

Thanks for working on this proposal. I think this proposal has some very good ideas, and I'm supportive of the direction overall, but I think it might be worth rethinking some aspects of it.

My main concern is that that this proposal would substantially increase the amount of ESLint-specific knowledge required for end users to perform some common tasks, such as extending a shareable config. In general, this proposal seems like it's taking a lot of the complexity of ESLint's shareable config resolution and throwing it on to the end user. This has some advantages (for example, configs are more expressive and composable with less "magic behavior" from ESLint), but I think it will also make it substantially more difficult for users to build configs. It seems like we agree that ESLint has some abstractions that are overcomplicated and cause problems, but in some cases I think this proposal is throwing the baby out with the bathwater by removing the abstractions completely.

This reminds me of Webpack config files. The Webpack config file format is very expressive and (as far as I'm aware) logically consistent, but in my experience it's also very difficult to understand what's going on in a Webpack config file without substantial background knowledge. I think it would be worth considering whether we could mitigate that type of effect through better API design.

The proposal does a lot of things, so I'll address them individually:

Removing extends

extends actually does two separate things at the moment: it loads another config from the filesystem, and it merges that config with the parent config. This proposal removes both functionalities, but I think it's important to distinguish between them, because they have very different tradeoffs.

  • Removing the behavior where ESLint loads another config from the filesystem is a good choice; in fact, it's I think it's the main benefit of this proposal. The advantage is that the user is providing a single config object to ESLint, which is assumed to contain all necessary information. This prevents ESLint from needing to search through the filesystem for the user's dependencies, and makes it much easier for users to debug or programmatically modify their configs.

  • I don't think it's a good idea to remove the functionality where third-party configs are merged with parent configs as a core part of the config format. According to a recent study, 67.2% of projects use a shareable config (including eslint:recommended); this is an extremely common use case. While it's theoretically possible for users to merge shareable configs on their own, doing so seems very error-prone. (Case in point: the example given in this RFC has a bug in that it mutates the global state of a shareable config module, potentially causing confusing problems with integrations.) As a result, it seems like most users would either need to add the @eslint/config package as an additional dependency (and understand how to use it), or implement their own buggy version. Even when using the @eslint/config package, configs would still be flattened before ESLint sees them, resulting in a suboptimal debugging experience.

    Additionally, it's not clear what advantage is gained by removing the core config-merging functionality. We could remove the "loading configs from the filesystem" behavior without removing the "merging configs" behavior, by having configs contain something like this instead:

    extends: [
        require('eslint-config-airbnb')
    ]

    This would effectively result in configs having a tree structure, rather than a flat structure. The tree structure would be flattened by ESLint core at runtime to produce a final resolved config. I think this would improve the experience for users, because it would allow for a much simpler way to carry out a common use case, while also allowing the original config tree to be used for debugging (e.g. we could add debugging functionality to pinpoint where a particular rule is being configured).

    (We would still need to handle the issue of ruledefs with the same name, but that's a problem regardless of whether people are using @eslint/config to merge configs.)

Removing envs

I'm fine with this. Envs were already redundant with shareable configs anyway.

Removing plugins

In effect, this proposal seems like it would completely remove the plugin API. ESLint core would no longer be touching plugin objects directly. Instead, users would load plugin modules on their own and extract the relevant components. Users might continue to create packages that export objects with rules and processors keys, but only by convention.

I have mixed feelings about this because it seems like it would make it difficult to add features to plugins in the future. With this change, there would be no way for us to add a new plugin API without explicit opt-in from end users.

For an example of a case where this would be a problem, suppose we hypothetically wanted to add a "prelint" hook where a plugin has access to all of the filepaths that will be linted (for the benefit of tools like typecheckers that operate on multiple files at once) and provides some services to rules as a result. End user opt-in seems unnecessary for this change because the new services would only be visible to rules, but if we implemented this RFC there would be no way for a plugin to opt into the feature without making the end user change their config.

One option would be to replace ruledefs with something like pluginDefs, where the values of the object would be plugins themselves (and envs/configs keys would be ignored). On the other hand, this would require processor to be a string again, which doesn't seem ideal.

Removing overrides

This RFC removes overrides in favor of supporting an array of configs with explicit files arguments. I don't have a strong objection to this, although in my experience, most overrides make very small modifications to a larger config. Repeating the same config for multiple globs seems tedious (even if the config is assigned to a variable, it still adds some boilerplate), and it's not clear to me what the value-add is.

Replacing .eslintignore

This is fine by me, although it might be confusing if ignore patterns are inherited from third-party configs.

designs/2019-config-simplification/README.md Outdated Show resolved Hide resolved
designs/2019-config-simplification/README.md Outdated Show resolved Hide resolved
designs/2019-config-simplification/README.md Outdated Show resolved Hide resolved
designs/2019-config-simplification/README.md Outdated Show resolved Hide resolved
designs/2019-config-simplification/README.md Show resolved Hide resolved
designs/2019-config-simplification/README.md Outdated Show resolved Hide resolved
designs/2019-config-simplification/README.md Outdated Show resolved Hide resolved
designs/2019-config-simplification/README.md Outdated Show resolved Hide resolved
designs/2019-config-simplification/README.md Outdated Show resolved Hide resolved
designs/2019-config-simplification/README.md Outdated Show resolved Hide resolved
designs/2019-config-simplification/README.md Outdated Show resolved Hide resolved
designs/2019-config-simplification/README.md Show resolved Hide resolved
designs/2019-config-simplification/README.md Outdated Show resolved Hide resolved

exports.config = {
ruledefs: {
react: reactPlugin.rules
Copy link
Member

Choose a reason for hiding this comment

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

One thing to consider here is how the users will be able to search the internet for specific error they receive if they use shareable config and config author decided to rename the plugin. I.e.:

const reactPlugin = require("eslint-plugin-react");

exports.config = {
  ruledefs: {
    "not-angular": reactPlugin.rules
  },
  rules: {
    "not-angular/jsx-uses-react": "error"
  }
}

As far as I understand, with configuration like that, error on the console will be reported as not-angular/jsx-uses-react which will make it hard to find which plugin error came from. Also, for overriding rules, you have to know what the author of shareable config decided to name plugins as well, which means reading shareable config source code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, there is no more canonical name for a rule in this proposal. I'm not entirely sure that matters, though. I anticipate a lot of users would just use the plugin name as their namespace. We could encourage plugin authors to export a namespace property so there is a canonical name to refer to:

const reactPlugin = require("eslint-plugin-react");

exports.config = {
  ruledefs: {
    [reactPlugin.namespace]: reactPlugin.rules
  },
  rules: {
    "not-angular/jsx-uses-react": "error"
  }
}

Would that make sense?

Copy link
Member

Choose a reason for hiding this comment

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

I guess we could try that. But it requires person who creates a config to not only know about this property being exposed by plugin, but also know about computed property name syntax.

Copy link

Choose a reason for hiding this comment

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

could maybe we just pass in reactPlugin, only, and let the schema of that object be a contract maintained between plugins and eslint (ie, not involving end users)?

Copy link
Member Author

Choose a reason for hiding this comment

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

@ilyavolodin I think that if we help them with tools like --init then this becomes clearer. Eventually everyone will understand computed properties, so I'm not too worried about that.

@ljharb what type of contract are you envisioning? In this proposal, the concept for a formal plugin really goes away as you can pass any object that contains rules into ruledefs, so requiring plugins to look a certain way could restrict what otherwise is a wide-open playing field.

Copy link

@ljharb ljharb Jan 29, 2019

Choose a reason for hiding this comment

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

@nzakas You could start with "an object with a rules property", but then that allows for future backwards-compatible expansion, without requiring any config changes for users (only core changes, and then corresponding plugin changes).

Copy link
Member Author

Choose a reason for hiding this comment

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

Please see the FAQ I added about this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm adding a new section explaining how we can retain canonical rule names without making changes to this design.

designs/2019-config-simplification/README.md Outdated Show resolved Hide resolved
designs/2019-config-simplification/README.md Outdated Show resolved Hide resolved
designs/2019-config-simplification/README.md Outdated Show resolved Hide resolved
platinumazure and others added 4 commits January 23, 2019 07:42
Co-Authored-By: nzakas <nicholas@nczconsulting.com>
Co-Authored-By: nzakas <nicholas@nczconsulting.com>
Co-Authored-By: nzakas <nicholas@nczconsulting.com>
@nzakas
Copy link
Member Author

nzakas commented Jan 23, 2019

@not-an-aardvark Thanks for the thoughtful feedback. Here are some responses to your questions and concerns:

My main concern is that that this proposal would substantially increase the amount of ESLint-specific knowledge required for end users to perform some common tasks, such as extending a shareable config. In general, this proposal seems like it's taking a lot of the complexity of ESLint's shareable config resolution and throwing it on to the end user. This has some advantages (for example, configs are more expressive and composable with less "magic behavior" from ESLint), but I think it will also make it substantially more difficult for users to build configs. It seems like we agree that ESLint has some abstractions that are overcomplicated and cause problems, but in some cases I think this proposal is throwing the baby out with the bathwater by removing the abstractions completely.

My thoughts around this fall into a couple of categories:

  1. Config files are already quite complicated. Trying to figure out how configs are merged with extends, overrides, and cascading is difficult for people to understand. This is why we ended up creating the --print-config method, to give people a better idea of the magic they don't see.
  2. I think by removing all abstractions we can make a better decision about which ones are helpful and necessary and which aren't. Undoubtedly some things in this initial proposal won't work, but I didn't think we could keep most things and still make progress towards solving the problems we have.
  3. People don't modify config files frequently, so the pain of setting one up is front-loaded. We can mitigate that with --init as well.

This reminds me of Webpack config files. The Webpack config file format is very expressive and (as far as I'm aware) logically consistent, but in my experience it's also very difficult to understand what's going on in a Webpack config file without substantial background knowledge. I think it would be worth considering whether we could mitigate that type of effect through better API design.

I totally agree. That's part of why I think the addition of a helper package that makes things more explicit is key to this proposal.

The proposal does a lot of things, so I'll address them individually:

Removing extends

I don't think it's a good idea to remove the functionality where third-party configs are merged with parent configs as a core part of the config format.

Additionally, it's not clear what advantage is gained by removing the core config-merging functionality. We could remove the "loading configs from the filesystem" behavior without removing the "merging configs" behavior, by having configs contain something like this instead:

extends: [
     require('eslint-config-airbnb')
 ]

That's an interesting idea. Let me look at this proposal again to make sure it fits (I think it does) and if so, would eliminate a lot of complexity. (Just running out of energy now.)

Removing plugins

In effect, this proposal seems like it would completely remove the plugin API. ESLint core would no longer be touching plugin objects directly. Instead, users would load plugin modules on their own and extract the relevant components. Users might continue to create packages that export objects with rules and processors keys, but only by convention.

Keep in mind that we don't actually have a plugin API. What we have is a plugin format. ESLint has always treated plugins as just a collection of named things, and that doesn't change with this proposal. People could continue creating plugins in the exact same way, they'd just pluck out the configs, processors, and rules as necessary.

For an example of a case where this would be a problem, suppose we hypothetically wanted to add a "prelint" hook where a plugin has access to all of the filepaths that will be linted (for the benefit of tools like typecheckers that operate on multiple files at once) and provides some services to rules as a result. End user opt-in seems unnecessary for this change because the new services would only be visible to rules, but if we implemented this RFC there would be no way for a plugin to opt into the feature without making the end user change their config.

Our plugins don't do anything automatically. That's been a design principle from the start and I wouldn't expect that to change. Plugins could expose whatever functionality they wanted, including hook information, and people could decide whether or not to use it in their config. I'm not a fan of magic plugins that exert their will on users without explicitly opting-in.

One option would be to replace ruledefs with something like pluginDefs, where the values of the object would be plugins themselves (and envs/configs keys would be ignored). On the other hand, this would require processor to be a string again, which doesn't seem ideal.

I actually just used plugins earlier, but then realized this definition was only helpful for rules because parsers, processors, etc., could be passed objects. Then I changed it to ruledefs. I think in the case of your hypothetical, we could do the same thing with hooks. (Though I'd prefer not to go down this hypothetical road any further so we can focus on the current problems we have.)

Removing overrides

This RFC removes overrides in favor of supporting an array of configs with explicit files arguments. I don't have a strong objection to this, although in my experience, most overrides make very small modifications to a larger config. Repeating the same config for multiple globs seems tedious (even if the config is assigned to a variable, it still adds some boilerplate), and it's not clear to me what the value-add is.

In this case, you can create a base config that has everything you want and merge that into override configs so you still only modify a small number of things.

Replacing .eslintignore

This is fine by me, although it might be confusing if ignore patterns are inherited from third-party configs.

Agreed. That's a bit of a problem and hopefully we can figure out some easy way to deal with this going forward.

@nzakas
Copy link
Member Author

nzakas commented Jan 23, 2019

Thanks for the awesome feedback everyone. I'm out of energy for today, but I'll take a look at incorporating more feedback tomorrow.

I think the overall negative feedback I heard was that extends needs to be easier. I think @not-an-aardvark's suggestion for continuing to support extends with objects instead of strings makes sense, and my priority will be to incorporate that into this proposal next.

And sorry for the poor formatting and all-red text. :(

@ilyavolodin
Copy link
Member

I share @not-an-aardvark concerns about shifting complexities from us to end user, but for a different reasons, I think. There are so many people who are using ESLint, and, while some of them are developers with a lot of experience and understanding of the language, a large portion of the user base probably either just starting out with the language, or not up to date on the latest format. Merging configs manually is reasonably straight-forward if you know and understand ES6+ syntax changes like spread and rest, but if you've been stuck doing ES3/5 and don't know anything about spread, it's a lot of complex code that you have to write to merge multiple configs together.
One of the most common issues that we currently have is users not understanding the difference between global and local npm installation, which is reasonably trivial thing to figure out (at least, in my opinion it's a lot easier to understand that spread/rest operators in ES6+). So my concern is that a lot of users will not be able to create good enough configs for their projects.
And while @nzakas is correct, that the current configs are complicated as well - they are complicated for complex scenarios, for simple things like using shareable configs and changing couple of rules, they are actually pretty simple and straight-forward.
P.S. That's not to say that I don't like the proposal. I just think that the focus is to make more complicated configs simpler, at the expense of making simple configs more complicated.

@platinumazure platinumazure dismissed their stale review January 23, 2019 16:22

Concerns have been addressed but more iteration is needed

@mysticatea
Copy link
Member

@nzakas @not-an-aardvark how about my proposal in #9 (comment) ? It will simplify the config system with merging extends and overrides functionality. I think linear overriding is more understandable than combination of the two.

@nzakas
Copy link
Member Author

nzakas commented Jan 24, 2019

I've updated the proposal to add back extends, per @not-an-aardvark's feedback. I do think that makes this proposal a lot more appealing that the original and reduces a lot of complexity for end users.

Copy link

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Can you add an example (or point to it if I missed it) about where in the config we'd define --ext?

designs/2019-config-simplification/README.md Outdated Show resolved Hide resolved

exports.config = {
ruledefs: {
react: reactPlugin.rules
Copy link

Choose a reason for hiding this comment

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

could maybe we just pass in reactPlugin, only, and let the schema of that object be a contract maintained between plugins and eslint (ie, not involving end users)?

@nzakas
Copy link
Member Author

nzakas commented Jan 25, 2019

Pushed more updates:

  • Removed Config.create() (not necessary now that we have extends back)
  • Added example for replacing --ext

@nzakas
Copy link
Member Author

nzakas commented Jul 10, 2020

As this RFC has been in final commenting for over a week with no further issues uncovered, and the TSC has already approved this RFC, I'm merging.

I've created this issue to track implementation progress of this RFC:
eslint/eslint#13481

Thanks to everyone for your patience and feedback. It's exciting to finally get started building this!

@nzakas nzakas merged commit fa9bb2a into master Jul 10, 2020
@nzakas nzakas deleted the newconfig branch July 10, 2020 16:06
@tunnckoCore
Copy link

Hey there. I'm here again. Read through the spec again and yet this thing is still not clear (wondering since then when I was tracking):

image

In the first sentence you say that there must only be one config (module.exports being array or just a single config) that has files.

But in the second, you say that every config that don't have files is merged into every that has files.

Which one is true? If it's the first one, it's easy - filter configs that don't have files, merge them, then run linting/globbing on every config that has files.

Another thing is, what if files is an array of multiple strings, and not [ ['*.test.*', '*.js'] ] which means match every pattern? Is it acting with the usual behavior for such things? Like files: ['*.test.*', '*.js'], will mean either *.test.* or *.js.

cc @nzakas

@haltcase
Copy link

@tunnckoCore it seems this thread from quite a while ago shared your concern. While the current phrasing is technically correct, it is potentially confusing.

I assume it's intended to mean that at least one config object must have the files property — "only one must" rather than "there must be only one".

@tunnckoCore
Copy link

@haltcase thanks, got it.

btw, your link points ro this same thread, not another one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Final Commenting This RFC is in the final week of commenting
Projects
None yet
Development

Successfully merging this pull request may close these issues.