-
-
Notifications
You must be signed in to change notification settings - Fork 70
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
New: Load configs and plugins relative to where they're referenced #5
Changes from 3 commits
88de0b2
a043b9d
282edac
a1a271f
af5aeeb
d2ea034
d0d8474
d62e3e8
6e5ce88
2a26810
5565990
996fe9a
dbf696d
4f0c43a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,6 +65,7 @@ In this example, the end user's config extends `eslint-config-foo` and `eslint-c | |
* 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.) | ||
* In a rule configuration like `foo::bar::react/no-typos`, the config scope is `['foo', 'bar']`, the plugin name is `react`, and the rule name is `no-typos`. | ||
* The syntax shown here for writing a config scope (which uses `::` as a separator) is only a minor detail of this proposal, and is open to bikeshedding. For the purposes of understanding this proposal, it's recommended to focus on the abstract idea of a `(configScope, pluginName, ruleName)` triple; the question of how best to syntactically represent that idea can be decided independently of the rest of the proposal. | ||
* Note: Another syntax was considered which uses `/` characters for all separators, e.g. `foo/bar/react/no-typos`. However, this would lead to parsing ambiguity because some rule names have slashes in them, so it would be unclear whether the reference refers to a `react` plugin with a `no-typos` rule, or a `bar` plugin with a `react/no-typos` rule. | ||
* Each reference to a plugin rule is also implicitly associated with a config in the config tree. References that appear in a config file are associated with that config file. References outside of a config file (e.g. from the command line or inline config comments) are associated with the root of the config tree. | ||
|
||
To resolve a `(configScope, pluginName, ruleName)` triple to a loaded rule, which is referenced in a config `baseConfig`: | ||
|
@@ -106,6 +107,23 @@ Using the config tree given above (reproduced below for convenience): | |
* With this proposal, there is no longer any behavioral distinction between a "local" and a "global" installation of ESLint. Since packages are no longer loaded from the location of the ESLint package, ESLint generally behaves the same way regardless of where it's installed. (As a minor caveat, ESLint still resolves references to core rules by using the rules that are bundled with it, so if a user has different *versions* of ESLint installed globally and locally, the behavior might still vary depending on which version of ESLint is run.) | ||
* For configs loaded via `extends` with a relative path, via `.eslintrc.*` files in nested folders, the location of the "base" config file is used to load plugins and shareable configs. This is a minor detail to address the unusual case where a relative `extends` clause crosses a `node_modules` boundary, which would otherwise allow the same plugin name to resolve to two different plugins without any shareable config reference that could be used to disambiguate them. | ||
|
||
### Implementation notes | ||
|
||
#### Resolving modules | ||
|
||
The task of resolving modules from a particular location will likely be accomplished using Node's `Module._findPath` API, similar to how it's [already used in the codebase](https://github.com/eslint/eslint/blob/62fd2b93448966331db3eb2dfbe4e1273eb032b2/lib/util/module-resolver.js). | ||
|
||
Node's module caching is not expected to pose an issue, because module caching never causes a different version of a package to get loaded. Its only effect is that if the *same* package is loaded from two different sources, both sources might load the same JavaScript object. | ||
|
||
* If two shareable configs depend on different versions of a particular plugin, then two separate versions of the plugin package will be loaded, even though they share a package name. | ||
* If two shareable configs depend on the same version of a particular plugin, then load the plugin from the two locations may or may not create the same JavaScript object, depending on how the package manager flattens packages. This should not cause a problem in either case, because ESLint doesn't mutate the plugin objects that it loads. | ||
|
||
#### Effect on existing codebase | ||
|
||
The bulk of the implementation will likely involve rewriting large portions of [`lib/config/config-file.js`](https://github.com/eslint/eslint/blob/62fd2b93448966331db3eb2dfbe4e1273eb032b2/lib/config/config-file.js) to ensure that it builds a tree of configs as described above, rather than repeatedly merging everything into a single config. | ||
|
||
The implementation is not expected to affect config caching logic; configs can be cached from the filesystem in the same manner that they are today. The implementaiton should only change what happens with a config after it's loaded from the filesystem, regardless of whether the filesystem access was cached. | ||
|
||
## Documentation | ||
|
||
This proposal has a few backwards-incompatible aspects, so it would appear in a migration guide for the major version where it's introduced. It would also entail updating plugin documentation to suggest adding shareable configs as dependencies, and removing documentation about the difference between local and global ESLint installations. | ||
|
@@ -215,16 +233,37 @@ This proposal maintains compatibility for most shareable configs, and most local | |
|
||
## Alternatives | ||
|
||
* 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. 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. | ||
* [eslint/eslint#3458 (comment)](https://github.com/eslint/eslint/issues/3458#issuecomment-257161846) proposed solving the duplicate-name problem by using plugins that depend on other plugins and reexport the rules of their dependencies, without any changes to ESLint core. It suggested two possible ways of re-exporting the rules: either a plugin could export them directly with the same name, or it could give the names a common prefix. This was proposed to address the issue that end users are exposed to their configs' dependencies. | ||
### Raise a fatal error for duplciate plugin names | ||
not-an-aardvark marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
### Use plugins that pull rules from other plugins, instead of shareable configs | ||
|
||
[eslint/eslint#3458 (comment)](https://github.com/eslint/eslint/issues/3458#issuecomment-257161846) proposed solving the duplicate-name problem by using plugins that depend on other plugins and reexport the rules of their dependencies, without any changes to ESLint core. It suggested two possible ways of re-exporting the rules: either a plugin could export them directly with the same name, or it could give the names a common prefix. This was proposed to address the issue that end users are exposed to their configs' dependencies. | ||
|
||
Unfortunately, this solution has a few downsides that prevented it from being widely adopted: | ||
|
||
* It would require everyone using shareable configs to switch to using plugins, which was regarded by some config authors as an unacceptably large breaking change. | ||
* It could create confusion about where a rule came from (and where bugs should be reported), because a rule might have been passed through many different plugins before reaching the user. | ||
* It still caused issues if two loaded plugins exported rules with the same name, resulting in either (a) a naming conflict where one rule would be unconfigurable, or (b) scoped rule names where the end user would end up exposed to their config's dependencies anyway. | ||
* This solution would not fix the issue that ESLint depends on package managers' implementation details (at least partly because that issue was not known at the time that the solution was proposed). | ||
|
||
### Recommend that shareable configs install their peer dependencies with a postinstall script | ||
|
||
Another possible course of action would be to encourage shareable configs to install their peer dependencies with a `postinstall` script. This would avoid adding complexity to ESLint core, while preventing users from needing to manually install peer dependencies. | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
* 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I strongly disagree. I think there's an important distinction between:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. So:
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 |
||
|
||
This solution has similar effects to the solution of raising an fatal error for duplicate plugin names. Both avoid the complexity of hierarchical rule name resolution, but they offer no good user recourse if conflicting plugin versions are used. Since the "disallow duplicate plugin names" solution also fixes the bug where ESLint fails with some package management setups, it seems like a better fallback solution than recommending a postinstall script, if hierarchial rule name resolution is determined to add too much complexity. | ||
|
||
Unfortunately, this solution has a few downsides that prevented it from being widely adopted: | ||
### Do nothing | ||
|
||
* It would require everyone using shareable configs to switch to using plugins, which was regarded by some config authors as an unacceptably large breaking change. | ||
* It could create confusion about where a rule came from (and where bugs should be reported), because a rule might have been passed through many different plugins before reaching the user. | ||
* It still caused issues if two loaded plugins exported rules with the same name, resulting in either (a) a naming conflict where one rule would be unconfigurable, or (b) scoped rule names where the end user would end up exposed to their config's dependencies anyway. | ||
* This solution would not fix the issue that ESLint depends on package managers' implementation details (at least partly because that issue was not known at the time that the solution was proposed). | ||
* A final alternative would be to do nothing. This would avoid all compatibility impact, but also leave ESLint unable to be used in certain package management setups, and users would likely continue to be confused about why their plugins sometimes aren't found. | ||
A final alternative would be to do nothing. This would avoid all compatibility impact, but also leave ESLint unable to be used in certain package management setups, and users would likely continue to be confused about why their plugins sometimes aren't found. | ||
|
||
## Open Questions | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct.
react/no-typos
remains valid unless there is an ambiguity, in which case something likefoo::react/no-typos
must be used to disambiguate.