New: Load configs and plugins relative to where they're referenced #5
Conversation
Co-Authored-By: not-an-aardvark <teddy.katz@gmail.com>
Co-Authored-By: not-an-aardvark <teddy.katz@gmail.com>
There was a problem hiding this comment.
While there's a lot of detail in this proposal about how things would work at a high level, it looks like there isn't any implementation detail. That's what I'm most interested in, and would like to see added to this RFC. And specifically, how will you load two different versions of the same plugin without running into caching problems with require?
My overall concern with this proposal is that we are introducing a lot of complexity, most of which will be pushed onto end-users, in the name of what can typically be worked around if shareable configs use install-peers in their post-install script. That effectively mimics how installing a shareable config would have worked before npm changed their behavior to not automatically install peer dependencies. It seems like asking shareable config authors to do so correctly places the burden on shareable config authors instead of on end-users.
|
@nzakas Thanks for the feedback. I've added the sections you recommended. I've also added the |
|
|
||
| In the case where `eslint-config-foo` and `eslint-config-bar` both end up loading the exact same copy of `eslint-plugin-react` (e.g. due to a package manager's tree flattening), ESLint could avoid the performance cost of running the same rule twice by simply running the rule once and producing two reports. (It doesn't seem like a good idea to only output one report in this case, because this would make ESLint's output highly dependent on how a package manager decides to arrange packages in `node_modules`.) | ||
|
|
||
| The UI impact of this problem could largely be mitigated by updating formatters to deduplicate reports when displaying them. For example, rather than displaying two separate lines for two reports with the same message at the same location, a formatter could display the message once and list both rule names. |
There was a problem hiding this comment.
I agree it's best to handle this in formatters. Should we update our default formatter to dedupe as part of implementing this?
There was a problem hiding this comment.
I don't think it makes sense to do this in formatters because every formatter would have to be updated to handle duplicates (both core formatters and community ones). Couldn't this be done as part of the results processing we already do before returning results to end users?
There was a problem hiding this comment.
There are a few possible ways this could be done as part of results processing. I decided that both have substantial drawbacks that prevent them from being worthwhile, but let me know if you have other thoughts:
- As part of results processing, we combine duplicate reports into a single report, and change the rule ID to account for the fact that the report came from multiple rules (so the rule ID could be the string
"foo::react/no-typos, bar::react/no-typos"). This seems like it would break a lot of integrations, which rely on theruleIdproperty corresponding to a single, existent rule. - As part of results processing, we remove all but one copy of each report (so a report could come from a rule ID of
bar::react/no-typos, and no report would be shown forfoo::react/no-typoseven though that rule also reported an error). This seems like it could create a confusing user experience where disabling one rule would unexpectedly cause new reports to appear from another rule.
|
|
||
| If a user extends `eslint-config-foo` and `eslint-config-bar`, and `eslint-config-foo` depends on `eslint-plugin-react`, then `eslint-config-bar` cannot reference the version of `eslint-plugin-react` used by `eslint-config-foo`. (If `eslint-config-bar` wanted to use `eslint-plugin-react`, it could instead install its own version of `eslint-plugin-react`, independently of `eslint-config-foo`.) | ||
|
|
||
| This property is by design; since `eslint-config-bar` can't know what version of `eslint-plugin-react` is used by its siblings (or if `eslint-plugin-react` is in use at all), it generally wouldn't know how to produce a reasonable configuration for the rules in `eslint-plugin-react`. However, this will break some aspects of shareable configs like [`eslint-config-prettier`](https://github.com/prettier/eslint-config-prettier), which attempts to disable all stylistic rules from a set of a few popular plugins. (The particular issue of disabling stylistic rules might be solvable separately from this proposal since rules can now be explicitly marked as stylistic through the `type` metadata flag.) |
There was a problem hiding this comment.
I'd expect some pushback from breaking eslint-config-prettier. Using npm downloads as a proxy, it's included in 17% of eslint installations. Is there some way we could provide a smooth migration?
There was a problem hiding this comment.
To clarify, this would only break the plugin-specific configs bundled in eslint-config-prettier (namely prettier/flowtype, prettier/react, prettier/standard, prettier/unicorn, and prettier/vue), not the main config which addresses core rules. I don't have empirical data about the usage of these configs, but I suspect their usage is significantly smaller. (In any case, I agree we shouldn't take the breakage lightly.)
As part of the discussion on adding the --fix-type flag, we also considered the possibility that it could be used to replace eslint-config-prettier. I think an ideal solution would be to provide a migration path by adding something like that to core.
Other possible migration paths include (ranked roughly in order of preference, although they aren't mutually exclusive):
- Plugins could expose configs that disable all of their stylistic rules. Then a user could use something like
extends: ['airbnb', 'plugin:airbnb::react/disable-all-stylistic-rules']to disable all the stylistic rules that were originally enabled byeslint-config-airbnb. - Using wrappers around shareable configs that disable stylistic rules. The use case for
eslint-config-prettieris usually that stylistic rules have been enabled by another shareable config (e.g.eslint-config-airbnb), and the user wants to disable all of those stylistic rules. So instead of enumerating rules from popular plugins,eslint-config-prettiercould instead expose configs that wrap popular shareable configs, e.g. aprettier/airbnbconfig which is the same aseslint-config-airbnbexcept that it disables all stylistic rules. Then an end user would usethe prettier/airbnbconfig as a substitute for theairbnbconfig. - Using scripting outside of the ESLint config extension process. For example, a package could expose a function that accepts the end user's config object and adds
foo: "off"to the rules section (substituting the appropriate rule names for"foo"). This could lead to ambiguity errors if multiple version of a plugin were installed, so it wouldn't be an ideal long-term solution, but it would provide a migration path for existing cases. - In ESLint core, we could allow something like
..in a config scope to refer to a parent in the tree (which would alloweslint-config-prettierto use something like..::reactto refer to its siblings). But this seems like it would break some invariants of the design and could result fragile cases, such as configs that depend on being used as siblings of other configs. This solution doesn't seem worthwhile just for compatibility with a single shareable config.
|
|
||
| In other words, in this case it would be possible to refer to the version of the plugin from `eslint-config-foo` with `foo::react/no-typos`, but there isn't anything that could be used in place of `foo::` if the user intended to refer to the direct dependency. Instead, the unscoped `react/no-typos` reference is allowed and refers to the direct dependency rather than being considered ambiguous. | ||
|
|
||
| One potential alternative would be to introduce a separate syntax to refer to direct dependencies of the current config file. For example, `::react/no-typos` (only a prefix of `::` with nothing before it) could refer to the direct dependency, and `foo::react/no-typos` could refer to the version from `eslint-config-foo`. The drawbacks of this alternative would be an increase in complexity and a loss of the invariant that a user can always use `pluginName/ruleName` to refer to plugins that they directly depend on, regardless of other things they have installed. |
There was a problem hiding this comment.
After reading the previous paragraph, I was going to suggest permitting an explicitly empty config scope with ::react/no-typos until I saw that you covered that here. But I agree that the drawbacks you mentioned of mandating the empty scope prefix aren't worth it. However, if this were implemented such that react/no-typos were the preferred form, I wouldn't be upset if the empty scope form ::react/no-typos also happened to work if someone wanted to be explicit.
Co-Authored-By: not-an-aardvark <teddy.katz@gmail.com>
| #### Details of hierarchical rule name resolution | ||
|
|
||
| * Each reference to a plugin rule in a config consists of three parts: a *config scope* (i.e. a list of configs), a plugin name, and a rule name. | ||
| * For example, in an existing rule configuration like `react/no-typos`, the config scope is an empty list, the plugin name is `react`, and the rule name is `no-typos`. (In existing rule configurations, the config scope is always an empty list.) |
There was a problem hiding this comment.
Point of clarification here: does this mean that react/no-typos is a valid way to refer to a rule in this proposal and that the :: syntax is only necessary as a means of disambiguation? (I'd much prefer this be the case vs. forcing a scope.)
There was a problem hiding this comment.
That's correct. react/no-typos remains valid unless there is an ambiguity, in which case something like foo::react/no-typos must be used to disambiguate.
|
|
||
| Suppose a user extends a shareable config (`eslint-config-foo`), and later on they want to extend another shareable config (`eslint-config-bar`) which uses the same plugin (`eslint-plugin-react`). | ||
|
|
||
| In this case, the user might already have a config disabling some plugin rules, e.g. `{ rules: { "react/no-typos": "off" }, "extends": ["foo"] }`. If they add `bar` to the `extends` list, then ESLint will report an error looking something like this: |
There was a problem hiding this comment.
This is a nice way of helping users along, but I fear most shareable config authors will just start using scoped rule configs as their recommended default to make sure that end-users don't ever see this message.
There was a problem hiding this comment.
To clarify: If react/no-typos appears in the shareable config, it will continue to work as before. This is because the declaration in the shareable config is in the shareable config's "scope" and isn't aware of anything outside the shareable config. Disambiguation would only be necessary if the consumer of the shareable config references the rule. This ensures that a shareable config can't be broken by adding a new config alongside it, so it wouldn't be necessary for the shareable config author to do this.
|
|
||
| In the case where `eslint-config-foo` and `eslint-config-bar` both end up loading the exact same copy of `eslint-plugin-react` (e.g. due to a package manager's tree flattening), ESLint could avoid the performance cost of running the same rule twice by simply running the rule once and producing two reports. (It doesn't seem like a good idea to only output one report in this case, because this would make ESLint's output highly dependent on how a package manager decides to arrange packages in `node_modules`.) | ||
|
|
||
| The UI impact of this problem could largely be mitigated by updating formatters to deduplicate reports when displaying them. For example, rather than displaying two separate lines for two reports with the same message at the same location, a formatter could display the message once and list both rule names. |
There was a problem hiding this comment.
I don't think it makes sense to do this in formatters because every formatter would have to be updated to handle duplicates (both core formatters and community ones). Couldn't this be done as part of the results processing we already do before returning results to end users?
|
|
||
| One alternative solution would be to avoid the complexity of hierarchical rule name resolution by simply raising a fatal error if two plugins have the same name. That solution is much simpler than this one, and avoids the duplicate report problem. Notably, implementing that solution first would also allow hierarchical rule name resolution to be added on later if necessary, without breaking compatibility. | ||
|
|
||
| However, with that solution the user has little recourse if two shareable configs both depend on the same plugin, resulting in a "dependency hell" scenario where many pairs of shareable configs would be incompatible with each other due to different dependency versions. |
There was a problem hiding this comment.
I would note that this situation was exactly what we had before npm changed the behavior of peer dependencies. If two configs had different versions of the same plugin as dependencies, they would end up needing to choose.
On some level, I think that's okay, because the issue is a dependency compatibility/versioning issue, and that really should fall under npm's responsibility rather than ESLint's.
There was a problem hiding this comment.
I disagree that this is npm's responsibility because our use case requires more than what npm is designed to solve. Specifically, the design of Node modules assumes encapsulation (if my module has a dependency on foo, I don't care what foo depends on as long as it exposes the API I need), whereas ESLint configs require deep dependency tree analysis (I depend on a shareable config which includes a plugin and enables its rules, I need to be able to arbitrarily override whatever it configured).
| Unfortunately, this would cause a few issues: | ||
|
|
||
| * If two shareable configs depended on different versions of the same plugin, their postinstall scripts would conflict with each other, resulting in one of the shareable configs having an incompatible peerDependency. This would require end users to manually remove a shareable config, and end users might end up confused about why they can't use two shareable configs together. | ||
| * With this solution, ESLint would continue to depend on implementation details of package managers, so it would still break under some valid setups (e.g. in `lerna` monorepos). |
There was a problem hiding this comment.
I think ESLint is already tightly coupled to how npm works, so I don't think this is a big concern.
There was a problem hiding this comment.
I strongly disagree. I think there's an important distinction between:
-
Coupling with the de-facto
package.jsonspec (i.e. assuming that if someone specifies a package independenciesand callsrequireon it, the package will be found). This is a reasonable place to be. -
Coupling with the specific implementation details of how a package manager implements the
package.jsonspec (i.e. assuming that if we callrequireon something, it will be found because someone else has a dependency on it). This is where we are now, and it has caused major problems where ESLint is sometimes inherently unable to load the user's packages if they're installed with Yarn Plug n' Play or Lerna. (For the full details on why this doesn't work, see Declaring shareable configs and plugins in package.json is unreliable eslint#10125.)This isn't just a distinction between npm and other package managers -- ESLint can also break or behave unexpectedly under certain circumstances even with npm, because we're not relying on supported behavior. (To illustrate: suppose the user decides they want to use
espree@3for some reason, so they addespree@3as a dependency and includeparser: espreein their config. Their code will still be parsed withespree@4because ESLint happens to depend on that package, and ESLint loads the user's parser from its own dependencies rather than the dependencies that the user can control.)
There was a problem hiding this comment.
I think it's appropriate to couple to how node works, however - and if the way some tools install packages breaks that, that's their breakage, and isn't something eslint necessarily needs to cleave to.
There was a problem hiding this comment.
I agree in the abstract, but I don't think that case is applicable here. Node's behavior depends on how packages are arranged in the filesystem, and the package manager is in charge of arranging the packages in the filesystem. package.json files are the way we express constraints on a package manager.
So:
- A package manager is correct if it arranges packages in a manner such that Node will able to load all the dependency relationships expressed in the
package.jsonfiles. - A library is correct (as far as dependencies are concerned) if it accurately expresses its dependency relationships in the appropriate
package.jsonfile(s).
The result is that if both the library and the package manager are correct, then Node will be able to load the library's dependencies. In this case, ESLint has dependencies that aren't expressed in a package.json file (namely, it needs to load user-provided plugins as if they were ESLint's own dependencies). So the bug is in ESLint, not the package manager.
|
|
||
| Unfortunately, this would cause a few issues: | ||
|
|
||
| * If two shareable configs depended on different versions of the same plugin, their postinstall scripts would conflict with each other, resulting in one of the shareable configs having an incompatible peerDependency. This would require end users to manually remove a shareable config, and end users might end up confused about why they can't use two shareable configs together. |
There was a problem hiding this comment.
Is this different than what happens today? I think the only difference is that the non-edge case (using just one config) becomes a lot easier while the edge case (of clashing config-required dependencies) remains mostly the same in that the end user has to figure it out.
There was a problem hiding this comment.
It's slightly different from a UX perspective in that the end user is more likely to be confused about why there's a problem, given that they weren't involved in installing the conflicting dependencies in the first place. It's the same in terms of where problems occur and who can fix them.
|
Something worth considering: it looks like npm is trying to solve part of the duplicate package problem in a way that might altogether prevent duplicate packages from being installed. It might be worth delaying any concerns over duplicate plugins until this is resolved one way or another. |
|
Please don’t wait on solving this years-old pain point in eslint based on waiting for a speculative feature that may not even make it into npm, and would necessitate people updating npm itself as well as updating everything in the ecosystem to support it - versus this solution where it requires only eslint, and the shared config, to make changes. |
|
What would be everyone's thoughts on implementing the suggestion from 4f0c43a? That would allow us to separate eslint/eslint#10125 from the issue of allowing shareable configs to manage plugins, while still allowing solutions like this RFC to be built on top of it if necessary. (I can open a separate RFC for that if there's support for it.) |
|
@ljharb my larger point is that (as I've stated before), I don't think it's ESLint's job to solve the duplicate plugin problem. That should be part of whatever package manager is in use. Working around package managers to solve this problem is a surefire way to shoot ourselves in the foot. @not-an-aardvark I think that part makes a lot of sense. I'm assuming that behavior would introduce some breaking changes? Can you explain what those would be? |
|
The main breaking change with the suggestion in 4f0c43a would be that global ESLint installations would generally require configs/plugins/parsers to be installed locally (similar to the way it works in this RFC), because the configs/plugins would be loaded relative to the end user's project regardless of where the ESLint package is located. Since 4f0c43a would slightly change how packages are loaded, there would also be a minor risk of breaking very unusual scenarios where someone rearranges |
|
I think that seems like a reasonable change in behavior (and arguably, people installing ESLint globally might expect this to be the behavior). I'd be 👍 for splitting this out into a separate RFC with the pros/cons outlined. I have other questions, but don't want to muddy the discussion on this RFC with them. |
nzakas
added
Initial Commenting
BREAKING CHANGE: This commit also moves eslint & prettier to peerDependencies. After this change, users will have to manually install these dependencies into their project root. Previously we listed the plugin as a dependency of this config, and that only works because package managers will hoist the plugin to project root. This trick is not reliable with current eslint implementation. This also resulted in a bug when the package manager failed to correctly hoist the plugin package, see vuejs/vue-cli#4310 More details can be seen at: - eslint/eslint#3458 - eslint/rfcs#5
|
Thank you all for the discussion in this thread and particularly @not-an-aardvark for putting the RFC together! Now that we've decided on a direction to take configs going forward in #9, the TSC decided today to close the other config-related RFCs so we can focus on the new format. You can follow along with that implementation work at eslint/eslint#13481. |
ESLint plugins used by this config must also be installed within the consuming project due to limitations of ESLint: eslint/rfcs#5
BREAKING CHANGE: This commit also moves eslint & prettier to peerDependencies. After this change, users will have to manually install these dependencies into their project root. Previously we listed the plugin as a dependency of this config, and that only works because package managers will hoist the plugin to project root. This trick is not reliable with current eslint implementation. This also resulted in a bug when the package manager failed to correctly hoist the plugin package, see vuejs/vue-cli#4310 More details can be seen at: - eslint/eslint#3458 - eslint/rfcs#5
This is the same proposal as eslint/eslint#10643, now in RFC form. A significant amount of detail has been added that wasn't present in eslint/eslint#10643 (just about everything starting at the "Documentation" section is new, as well as a few minor additions and restructuring before that).
Summary
This feature would update ESLint to plugins and shareable configs relative to the location of the config files where they're referenced, rather than relative to the location of the running ESLint instance. As a result, configs would be able to specify plugins as their own dependencies, and ESLint users would see fewer confusing errors about missing plugins.
Related Issues