-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Proposal for loading plugins relative to the configs that depend on them #10643
Comments
@not-an-aardvark this is an excellent breakdown of the core issue and proposed solution 👍 I've been following this issue for some time, and I can't see any problems with your proposed solution. This is such a ubiquitous issue that affects thousands of developers, it'd be great to gather consensus around this solution and move the ecosystem forwards soon. |
@not-an-aardvark This seems like a really solid proposal, thanks for thinking this through. I don't see any problems atm. Is there anything we can do to get more visibility on this PR so we can get more feedback and move forward with this? |
@not-an-aardvark What do we need to move forward with this proposal? Should we mark it for discussion with the rest of the TSC? I also have a few concerns about it below:
I'm not at all convinced that this is a real problem. Currently we require plugins to be valid NPM packages. Because of that, it's impossible to have two plugins with the same name. Now it is possible to have a config and a plugin with the same short-form name, but that shouldn't be an issue, since you can't really configure rules from the config. I know this requirement has been around for a very long time and was create by @nzakas but I honestly could never understand why we are trying to solve a non-existing problem. |
@ilyavolodin you could easily have two versions of the same plugin package. |
I'd be happy for more people to take a look at it. For now, if people have objections/concerns I think it might be more productive to discuss them on GitHub rather than at a TSC meeting. The complexity of this proposal in combination with the limited-time setting of a TSC meeting might make it difficult to address objections to the technical details. I think it would also be beneficial to have an implementation of this ready before we officially accept it at a TSC meeting; this would give a chance for users and TSC members to try it out and see how it works, and we could be aware of any problems that would pop up before officially making a decision on it. (I'd like to implement this at some point, but I haven't started yet.)
Thanks for the feedback and for voicing your concerns. It's true that we currently require plugins to be located in
It should be noted that for users who don't install plugins with the same name, the config complexity would be the same as it is currently. In other words, the new syntax is only necessary for resolving conflicts, so it doesn't need to be used if there are no conflicts. I agree that this would add implementation complexity in either case. I also think that this case wouldn't really be that rare, because many more shareable configs would include plugins after this change, increasing the chance of a conflict. (Currently, adding a plugin to a shareable config creates an installation burden for the user, so shareable configs often don't include them.) To me, it seems important that users can combine shareable configs without encountering fatal errors. |
I missed that part of the design proposal. Thanks for pointing that out. That does lower my concern about increased complexity of configuration. But still doesn't remove the increased complexity of the code on our side.
Problem is, it feels like we are trying to design a complex system without having any info/data on how it's going to be used. I think we are just guessing here about how often something like this is going to happen and how many people will run into this problem. I'm not sure how we can get more data on this subject without releasing the enhancement, but I also don't want to spend a whole lot of effort designing a fall-back strategy for something that might be very rare behavior with unexpected results.
Sure, I agree that might happen. I'm not sure, however, that's actually a desired behavior and makes sense from the user's perspective. At least right now, I can't come up with the scenario where something like this would make sense. |
I've been lurking. I care about StandardJS and I'm not finding any comments from the main people here so my apologies for the mentions in advance, but you probably should take a look at this, since eslint-config-standard is a popular shareable config that does depend on several plugins. |
Generally speaking, I want ESLint to be robust enough that it doesn't create failures for reasons outside of the user's control. I think throwing a fatal error for duplicate plugin names would be a poor solution because it would it would create a "dependency hell" scenario -- a shareable config would face pressure to not add a particular plugin as a dependency, because it would be incompatible with a user's other configs and the user would have no recourse except to stop using that shareable config. We have enough users that "rare" setups will still occur fairly often in terms of absolute numbers. Even if this only happened for 1% of config setups, we would still be creating problems for thousands of users. I think it's important that we give the user a way to resolve conflicts rather than just deciding not to handle the edge case. If this proposal fails, then I'd be willing to consider throwing a fatal error on conflicts as a backup, since it does have some significant advantages (and some disadvantages) over the current state of affairs. But I think this is a case where it's worth the implementation complexity to implement a more complete solution. |
That's a good argument. I'm just concern about added complexity for the end user when they run into an issue like that. Problem is, dependency on multiple plugin version will most likely never occur in the shareable config. It will always happen in the end-users config (at least that's my guess). And also on the maintenance burden on us for this extra feature. But I guess I can live with both, considering we've had this problem for number of years, and the is the first complete proposal that we have for it. |
@ilyavolodin if something like this proposal goes through, then eslint-config-airbnb very well could depend on another config, which could bring in its own plugins and config, which could create a conflict - and that level of composability is exactly what's already possible and awesome with npm, and it'd be great if shared configs could do the same. |
Note that this issue bites us in the context of Plug'n'Play, where we strictly enforce boundaries between packages. We do have a top-level fallback to account for this kind of plugin systems (if a package is required by a dependency that doesn't own it, we check whether the top-level dependencies list it and use it if it's the case), but it breaks apart in the case of shared configs (since in this case the top-level package usually doesn't list each individual plugin of the shared configs). The two solutions to this issue are either:
|
That seems like a flaw in PNP if it can’t account for normal resolution and dep nesting :-/ |
@ljharb It cannot because it's invalid, even right now. Let's take the following tree (nodes are packages rather than folders). Let's take the case of a package In typical Yarn installs, those three packages all get hoisted to the top-level in order to optimize the dependency tree. Everything's fine: Hoisting in as optimization. It may happen, but there's nothing that guarantees it. Even worse, relying on such a behavior means that your code will behave in unpredictable ways based on the other packages in your dependency tree - something that you don't control! For example, let's say that our top-level also depends on Plug'n'Play guarantees strict boundaries. We could decide not to do it, but I have to emphasis it: it does so by design. It prevents you from relying on Undefined Behaviors (in C/C++ terms), and as such makes your development more stable. Of course, tools relying on it to load plugins have a slightly hardest time, but the fallback coupled with |
Due to the issues in #10125, it's unsurprising to me that ESLint is currently failing with certain package management strategies, but I'm a bit confused about why this is happening with PNP. You mentioned that:
In fact, I think the current convention is for top-level packages to explicitly list the plugins for their shareable configs. Does the issue still occur when running ESLint from the top-level package, as opposed to from nested packages like |
If the top-level application lists all of the plugins, they will fall into the fallback, yep. It goes against the spirit of |
Yeah, in Create React App we're pretty intentional about ESLint being an implementation detail of our toolchain. We don't want our users to be exposed to a fact that there is a preset behind it until they "eject". |
I don't see the conflict; CRA can list all the preset's peer deps as deps, and that satisfies the peer dep requirements without hoisting anything up to the user's app itself. |
Understood. I was just trying to determine how much of the ecosystem is currently affected by this issue. @ljharb The issue is that ESLint's plugin-loading depends on Can we move this discussion to #10125? I think that issue has a better description of the problem, whereas this issue is more focused on a particular proposal. |
Hi everyone, The ESLint team has just created an RFC process for complicated changes that require designs. It seems like this discussion is now split amongst at least three different issues and would benefit from being consolidated into an RFC: https://github.com/eslint/rfcs/ I'd like to suggest that whomever is most interested (@not-an-aardvark?) please consolidate everything into an RFC proposal and close the outstanding issues so we can focus the conversation. Thanks! |
I've opened an RFC at eslint/rfcs#5. Closing this issue so that discussion can move there instead. |
(Previous discussions include: #3458, #10125)
Background/Problem description
Currently, ESLint plugins and shareable configs are loaded from the location of the ESLint package, rather than the location of the config file where the plugins and configs are referenced. This leads to several problems:
lerna
. (More details about this problem can be found in #10125.)To address these issues, many users have proposed loading all configs and plugins from the location of the config that depends on them (e.g. see #3458). For example, if a config has a field like
plugins: react
, ESLint would loadeslint-plugin-react
from the location of the config file itself. This would be beneficial because it would allow the author of a shareable config to control its dependencies, rather than the end user. It would also have a side-effect of removing the behavior change between local and global ESLint installations, which is a frequent source of confusion for users.Unfortunately, implementing this scheme as-is would cause naming ambiguity. There could be two shareable configs which have dependencies on two different plugins that both happen to be called
react
(or they depend on two different versions of a plugin calledreact
). If the end user depended on these two shareable configs and also configured a rule likereact/some-rule
in their top-level config, the end user's config would be ambiguous because it wouldn't be clear whichreact
plugin they were referring to. Since the configurations for a given rule might be incompatible across different versions of a plugin, this could make it impossible to set the configuration for a particular rule or to override a configuration which was set by a shareable config. This would be an unacceptably poor user experience.Another way to state the problem is that ESLint's mechanism for naming rules in a config file (
plugin-name/rule-name
) is fundamentally unable to disambiguate two plugins that have the same name. Currently, there are no naming conflicts because all plugins are loaded from the location of theeslint
package, so plugins effectively live in a global namespace. If plugins could be loaded as dependencies of shareable configs, then naming conflicts would start to become a problem. As a result, we need to be able to support having two plugins with the same name before we can support having plugins as dependencies of configs.Design goals of solution
package.json
spec, without relying on the implementation details of any particular package manager.peerDependencies
should be able to transition to the new solution without requiring changes to the configs of their users (or at least the vast majority of their users).Summary of proposed solution
Since the problem stems from a limitation of ESLint's naming scheme, I think the most straightforward solution to the problem would involve changing the way that rules are named in a config file. Specifically, this solution proposes using hierarchical naming for rules, where rules would be configured with something that looks like a path. (In other words, a user could refer to a rule like
foo::react/some-rule
for the version ofeslint-plugin-react
used byeslint-config-foo
, and this would be a different rule thanbar::react/some-rule
, which would refer to the version ofeslint-plugin-react
used byeslint-config-bar
.)This is similar to a few other proposals discussed in #3458, but I've done a significant amount of work since then on precisely describing the behavior, figuring out the edge cases, and thinking through alternatives.
Details of proposed solution
When describing how rule name resolution works in this proposal, it's useful to think of a "config tree" representing the dependencies between shareable configs and plugins. The root node is the end user's config, and each node has a set of named children representing the shareable configs that it extends and plugins that it depends on. Here's an example tree:
In this example, the end user's config extends
eslint-config-foo
andeslint-config-bar
.eslint-config-bar
extendseslint-config-baz
.eslint-config-foo
andeslint-config-baz
both depend on versions ofeslint-plugin-react
(perhaps different versions, although this doesn't matter as far as resolution is concerned).eslint-config-baz
also depends oneslint-plugin-import
.Rule name resolution
react/no-typos
, the config scope is an empty list, the plugin name isreact
, and the rule name isno-typos
. (In existing rule configurations, the config scope is always an empty list.)foo::bar::react/no-typos
, the config scope is['foo', 'bar']
, the plugin name isreact
, and the rule name isno-typos
.::
as a separator) is up for bikeshedding. For now, I would recommend focusing on the abstract idea of a(configScope, pluginName, ruleName)
triple; the question of how best to syntactically represent that can be decided independently of the rest of the proposal.To resolve a
(configScope, pluginName, ruleName)
triple to a loaded rule, which is referenced in a configbaseConfig
:configScope
is non-empty, find the child config ofbaseConfig
in the config tree which has a name ofconfigScope[0]
, and recursively resolve the rule(configScope.slice(1), pluginName, ruleName)
from that config.configScope
is empty:baseConfig
has a direct child plugin with the namepluginName
, orbaseConfig
is a plugin config from a plugin calledpluginName
, return the rule calledruleName
from that plugin.baseConfig
in the config tree and have a name ofpluginName
.ruleName
from that plugin.A few examples of this config resolution strategy, with the config tree given above (reproduced below for convenience):
Example config tree (same as above)
react/no-typos
, the config scope is empty. Since the root node of the tree has multiple descendants calledeslint-plugin-react
, the rule reference is ambiguous.bar::react/no-typos
, the config scope is non-empty, so the resolution strategy then tries to resolve the rulereact/no-typos
from theeslint-config-bar
node in the tree. Since there is only one descendent of that node calledeslint-plugin-react
, the rule would successfully resolve to theno-typos
rule of that plugin.Notable advantages and disadvantages of this strategy
plugins
array to override rule configurations from a shareable config's plugin; with this change, the shareable config and the end user's config would end up configuring two independent versions of the same plugin. To fix this in both versions, the end user's config could simply remove that plugin from theirplugins
array. There are also probably setups now where a shareable config overrides the plugin rules configured by a sibling shareable config; these could be fixed in both versions by making one shareable config a child of the other.eslint-config-foo
has two descendant plugins with the same name, then the config scope that is needed to refer to those rules is "contagious". In other words, ifeslint-config-foo
needs to use a reference likebar::react/no-typos
to avoid ambiguities, then a config that extendseslint-config-foo
needs to use something likefoo::bar::react/no-typos
to configure that rule.Support having plugins as dependencies in shareable config #3458 (comment) proposed adding an
exportedPlugins
field for shareable configs. This has an advantage that it makes the exposed API of a shareable config explicit. However, with this alternate proposal, ESLint would have to require each shareable config to export all of its dependency configs (otherwise the end user would be unable to reference rules from the dependency configs in order to override them). As a result, the dependency chain of a shareable config would still end up in an end-user config.Support having plugins as dependencies in shareable config #3458 (comment) proposed solving the problem by using plugins that depend on other plugins and reexport the rules of their dependencies, without any changes to ESLint core. It suggests 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. Unfortunately, both of these strategies have problems:
If these problems with alternative proposals seem like they would only occur in unlikely scenarios, I think it's worth noting that the problem in this proposal of having deep conflicting rule names in this proposal would also be somewhat unlikely. We should certainly make sure users have the ability to handle those scenarios, and give straightforward advice about how to do so (namely, that shareable configs need to bump the major version when adding plugins or adding dependencies that add plugins), but it's also important to make sure we make fair comparisons between proposals, and not compare the common cases of one proposal to the inconvenient edge cases of another.
I've spent a lot of time on this proposal, and would appreciate any feedback (particularly feedback that identifies problems with it). I think the current state of how plugins and configs are loaded is causing a large number of problems in the ecosystem, and I'm hoping we can work on this to finally pin down a solution.
The text was updated successfully, but these errors were encountered: