eslint / eslint

Find and fix problems in your JavaScript code.
https://eslint.org
MIT License
24.39k stars 4.4k forks source link

Support having plugins as dependencies in shareable config #3458

Closed sindresorhus closed 1 year ago

sindresorhus commented 8 years ago

My shareable config uses rules from an external plugin and I would like to make it a dependency so the user doesn't have to manually install the plugin manually. I couldn't find any docs on this, but it doesn't seem to work, so I'll assume it's not currently supported.

module.js:338
    throw err;
          ^
Error: Cannot find module 'eslint-plugin-no-use-extend-native'
    at Function.Module._resolveFilename (module.js:336:15)
    at Function.Module._load (module.js:278:25)
    at Module.require (module.js:365:17)
    at require (module.js:384:17)
    at /usr/local/lib/node_modules/eslint/lib/cli-engine.js:106:26
    at Array.forEach (native)
    at loadPlugins (/usr/local/lib/node_modules/eslint/lib/cli-engine.js:97:21)
    at processText (/usr/local/lib/node_modules/eslint/lib/cli-engine.js:182:5)
    at processFile (/usr/local/lib/node_modules/eslint/lib/cli-engine.js:224:12)
    at /usr/local/lib/node_modules/eslint/lib/cli-engine.js:391:26

I assume it's because you only try to load the plugin when the config is finished merging.

Other shareable configs that depend on a plugin instructs the users to manually install the plugin too and they have it in peerDependencies. I find this sub-optimal though and I don't want the users to have to care what plugins my config uses internally.

The whole point of shareable configs is to minimize boilerplate and overhead, so this would be a welcome improvement.

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

nzakas commented 7 years ago

@feross unfortunately, I don't think we can accept a solution that only works in a JS file with passing around objects. There are plenty of folks who don't use JS-based configurations, and we need to be able to support them, as well. We'd like to keep configs as data-only.

@stoikerty that's an interesting idea - we could do an intermediate step that allows you to pass in package by filepath. We'd have to do something to allow you to name that package (since we wouldn't have the package name. Something like what @feross mentioned:

plugins:
  foo: "/some/location/to/index.js"
rules:
  foo/rule: "error"

There is still a concern in that case that someone might include eslint-config-foo in a different config, and that would end up colliding with the foo defined here. Maybe we'd need some special naming convention to differentiate between foo meaning eslint-config-foo vs. foo meaning a custom name chosen for a filepath.

gaearon commented 7 years ago

There is still a concern in that case that someone might include eslint-config-foo in a different config

This might be ugly but could we consider an absolute filepath a valid plugin “identifier”?

const foo = require.resolve('eslint-plugin-foo');

module.exports = {
  plugins: [foo],
  rules: {
    [`${foo}/rule`]: 'error'
  }
}

We would not support relative filepaths which excludes any non-JS configs from supporting this feature, but presumably it's fine to make a subset of configs (JS-based ones) more powerful?

stoikerty commented 7 years ago

There is still a concern in that case that someone might include eslint-config-foo in a different config, and that would end up colliding with the foo defined here.

When eslint is parsing a config-file, is it able to distinguish between configs and their associated plugins?

Mapping it out, if you have the following: ⦿ pluginConfigA ➤ pluginConfigB ➤ userConfig ◉

where:

What is the most reasonable way to handle this scenario? Is it possible for each config to reference and use its own foo?

ilyavolodin commented 7 years ago

One other requirement that I wanted to mention is that the new way should be backwards compatible. We can't just break hundreds of plugins that are already published.

not-an-aardvark commented 7 years ago

I think the main issue here is that the current processing of config files doesn't allow contextual identifiers. In other words, eslint-plugin-import has to mean the exact same thing regardless of the config file that it appears in. This is very limiting, because if we want sharable configs to be usable independently from each other, they have to be able to have their own dependencies, and potentially their own versions of eslint-plugin-import.

To fix this, rules and plugins referenced in a config file should be resolved relative to the config file itself. In other words, config files will become more like modules rather than using a global plugin namespace.

Proposal:

For example, here's eslint-config-foo, which depends on eslint-plugin-bar@1.0.0. eslint-config-foo can specify eslint-plugin-bar as a dependency.

// ./node_modules/eslint-config-foo/index.js

module.exports = {
  plugins: ['bar'], // resolved as if require() was used from this file, yielding eslint-plugin-bar@1.0.0
  rules: {
    'bar/no-horses': 'error',

    // As before, eslint-plugin-foo can have local references to rules as well.
    'tball': './lib/rules/sports-jokes/tball.js'
  }
};

And here's eslint-plugin-bar@1.0.0:

// ./node_modules/eslint-config-foo/node_modules/eslint-plugin-bar/index.js

module.exports = {
  rules: {
    'no-horses': require('./lib/rules/bar-jokes/no-horses.js');
  }
};

Meanwhile, the user would like to use eslint-config-foo, but they would also like to use eslint-plugin-bar@2.0.0 separately. They can do this by installing eslint-config-foo and eslint-plugin-bar as dependencies (or devDependencies), and arranging their config file like this:

// ./.eslintrc.js

module.exports = {
  // `foo` is resolved by using require('eslint-config-foo') from this file
  extends: ['foo'],

  // `bar` is resolved by using require('eslint-plugin-bar') from this file.
  // This will yield eslint-plugin-bar@2.0.0, since the user has that installed as a dependency.
  plugins: ['bar'],

  rules: {
    // Again, references to `bar` are resolved by using `require` from this file.
    // So this will turn on the `tender` rule from eslint-plugin-bar@2.0.0:
    'bar/tender': 'error',

    // As before, `foo` is resolved by using require('eslint-config-foo') from this file.
    // This yields eslint-config-foo, which allows the user to configure rules from that config.
    'foo/tball': 'off',

    // Finally, a slash character can be used to chain references.
    // In this case, `foo` is resolved by using require('eslint-config-foo') from this file.
    // Then `foo/bar` is resolved by using require('eslint-plugin-bar') from eslint-config-foo.
    // This allows the user to toggle the rules from eslint-plugin-bar@1.0.0 that are used by foo:
    'foo/bar/no-horses': 'warn'
  }
};
BYK commented 7 years ago

@feross unfortunately, I don't think we can accept a solution that only works in a JS file with passing around objects. There are plenty of folks who don't use JS-based configurations, and we need to be able to support them, as well. We'd like to keep configs as data-only.

Can we offer this as a JS-only feature to at least get started? I understand the concern around restricting configs to only data but if we had the actual JS and module system, it may open other doors. Existing JSON and YAML configurations are already compatible with this, just more limited and in the future if we decide to drop support for these, it should be trivial to write a tool (or may be even include it in eslint) that converts these to JS modules.

not-an-aardvark commented 7 years ago

@BYK I think it's worth figuring out a good solution first, rather than implementing if halfway and figuring out the rest later. If we implement it halfway, it will make it much harder to change the system later.

gaearon commented 7 years ago

@not-an-aardvark

It is not clear to me from the team responses so far whether making JS configs more powerful is considered implementing it “halfway”, and why.

This is a hard problem if you treat configs purely as data, precisely because data presumes constant plugin identities whereas the issue is about potentially conflicting plugins.

Other tools, such as Babel, have already successfully solved the exact same problem by making presets code rather than data. I don’t quite understand why ESLint can’t follow the same path, even while allowing backward compatibility with “less powerful” formats. I also don’t quite see why having multiple formats to specify configs is useful, but this is probably due to my ignorance about the topic.

not-an-aardvark commented 7 years ago

@gaearon My objection was mostly in response to offering as a JS-only feature "to at least get started". If we determine that JS-only is the solution we want to use, then that's fine, but I don't think we should rush to a temporary solution with plans to change it later, because that will cause backwards-compatibility issues.

I think the solution in https://github.com/eslint/eslint/issues/3458#issuecomment-253968468 would be better anyway -- it would allow locally-resolved dependencies without needing to explicitly call require.

nzakas commented 7 years ago

Can we offer this as a JS-only feature to at least get started?

I thought I'd addressed this pretty clearly earlier in the thread, but I'm going to say it once again: no. Continuing to discuss strategies that only work in JS configs is not helping us solve the problem. I'm not going to go into a long explanation of why we have different config file formats, but the fact is that we do and we have a large ecosystem using all different types of formats. Our promise has always been that the formats are interchangeable, and I don't see that changing anytime soon. So let's focus on the problem at hand.

@not-an-aardvark That's an interesting approach, and I really appreciate you taking the time to explain it. Some points that popped into my head while reading it:

  1. It's unclear to me if this approach is breaking. Right now, if someone is using a shareable config that relies on a plugin, they are installing both the plugin and config, presumably at the same level. However, the shareable config has plugins: ['bar'] and the user config does not even though it's configuring rules like bar/rule. I think your proposal would break those configs (Airbnb is the most popular shareable config, and they are setup like this).
  2. There's a level of increasing complexity here that I'm concerned about: what if a shareable config is dependent on another shareable config that, in turn, is dependent on a plugin? How does that affect how you configure rules? (eslint-config-a depends on eslint-config-b which depends on eslint-plugin-p, do rules now become a/b/p/rule?)
  3. Keep in mind that plugins can also export configs that can be inherited from (extended). That means you can't always assume that one member of a slash-delimited identifier is a config vs. a plugin.

I think my overall feedback is that there may be a breakdown with the naming convention once we get into more involved relationships between shareable configs and plugins. Do you have thoughts around that?

not-an-aardvark commented 7 years ago

It's unclear to me if this approach is breaking. Right now, if someone is using a shareable config that relies on a plugin, they are installing both the plugin and config, presumably at the same level. However, the shareable config has plugins: ['bar'] and the user config does not even though it's configuring rules like bar/rule. I think your proposal would break those configs (Airbnb is the most popular shareable config, and they are setup like this).

There are two issues to consider here:

// ./node_modules/eslint-config-foo/index.js

module.exports = {
  plugins: ['bar'],
  rules: {
    'bar/rule1': 'error'
  }
};
// ./.eslintrc.js

module.exports = {
  extends: ['foo'],
  rules: {
    // eslint-config-foo had a valid reference to `bar`, and no valid reference can be resolved here.
    // As a fallback, this reference will work as an alias for 'foo/bar/rule1', resolving as the version of
    // `bar` that eslint-config-foo used.
    'bar/rule1': 'warn'
  }
};

While this would preserve backwards-compatibility, it isn't an ideal solution, since it could cause confusing behavior if the user installs something else that depends on a different version of eslint-plugin-bar. If we did decide to follow this pattern, we would probably want to deprecate it in favor of explicitly listing foo/bar/rule1 instead. (Alternatively, I'm open to hearing other workarounds for this.)

There's a level of increasing complexity here that I'm concerned about: what if a shareable config is dependent on another shareable config that, in turn, is dependent on a plugin? How does that affect how you configure rules? (eslint-config-a depends on eslint-config-b which depends on eslint-plugin-p, do rules now become a/b/p/rule?)

Yes, this would be resolved as a/b/p/rule. This does lead to increasing complexity, but I'm not sure this is avoidable; if there are there are multiple different versions of eslint-plugin-p scattered throughout node_modules, it is necessary to explicitly specify which version of eslint-plugin-p the user is referring to.

We could allow shorthands for these references if they're unambiguous (see the backwards-compatibility workaround above), which would allow things like p/rule rather than a/b/p/rule. However, I don't think we should encourage this pattern, since it's liable to unexpectedly break if a different version of eslint-plugin-p gets installed by some other dependency.

Keep in mind that plugins can also export configs that can be inherited from (extended). That means you can't always assume that one member of a slash-delimited identifier is a config vs. a plugin.

True, but I don't think this is a problem; we can simply resolve extended config files the same way as everything else. For example: 'airbnb/airbnb-base/import/no-unresolved': 'off'

I think my overall feedback is that there may be a breakdown with the naming convention once we get into more involved relationships between shareable configs and plugins. Do you have thoughts around that?

I would say that the naming convention works in these relationships, but it doesn't produce an ideal experience for users that want to configure rules from inherited plugins. Admittedly, having to use a path like a/b/p/rule in your config file is annoying.

However, if we want to allow multiple different versions of configs at the same time (which is necessary for a good resolution to this issue), while still allowing users to individually configure inherited rules, I don't think this annoyance is avoidable in the general case. Allowing multiple versions of a plugin to be run simultaneously will require the user to be more specific about which version of the plugin they want to configure.

nzakas commented 7 years ago

Yes, to clarify, I have no problem with the part of the proposal to load plugins relative to shareable configs. In fact, I've never been opposed to that, my only problem has been with how we expect users to address multiple versions of the same plugin in their configs -- to me, that's the hard part of this problem. (Although loading plugins relative to their configs is a nontrival effort to implement, it's pretty straight forward.)

I would say that the naming convention works in these relationships, but it doesn't produce an ideal experience for users that want to configure rules from inherited plugins. Admittedly, having to use a path like a/b/p/rule in your config file is annoying.

Yeah, I think this is a legitimately horrible experience. The problems (off the top of my head):

  1. A lot of cognitive overhead for end users.
  2. It brings the dependency chain of a shareable config into the end-user config. Exposing dependency information like that invites future compatibility problems if a dependency changes or is renamed.
  3. Still doesn't address the fact that currently, rule namespaces are only plugin names (namespace/rule). We'd need a different syntax for namespaces that are config names to avoid naming collisions.

If we are making the argument that shareable configs should work like any other modules, then I think the only real direction is to force shareable configs to re-export whatever it is that they want to expose to end users. That way, the end user only needs to know about the shareable config without any of its dependencies (which is ultimately what we want). So maybe a shareable config has something like:

exportedPlugins:
   foo: react       # Load eslint-plugin-react into identifier foo
   bar: import    # Load eslint-plugin-import into identifier bar

Then in end-user config, you'd do:

extends: myconfig
rules:
   $myconfig/foo/rule1: "error"
   $myconfig/bar/ruleA: "warn"

Where the $ indicates that this is coming from the config rather than a plugin. (Could be any special character, doesn't have to be $.)

The problem with this approach is that it only really works one level down. If a shareable config A depends on another shareable config B, I'm not sure how you'd say to promote the rules from B so they can be accessed from A.

As I've said a few times, the devil is in the details on this effort. :)

ljharb commented 7 years ago

Perhaps you could also do:

exportedPlugins:
   foo: react       # Load eslint-config-airbnb into identifier foo
   bar: import    # Load eslint-plugin-import into identifier bar
   baz: import/dep # Load eslint-plugin-import's exported plugin "dep" into identifier baz

In other words, it'd be one level down, but at each level, it could be explicitly re-passed?

not-an-aardvark commented 7 years ago

If we are making the argument that shareable configs should work like any other modules, then I think the only real direction is to force shareable configs to re-export whatever it is that they want to expose to end users

I'm not sure I understand this point. I thought a design goal here was that users should always be able to disable a given rule in their config. If we're okay with a rule being impossible to disable from a config since it's not explicitly exported by a plugin, then this changes some of the strategies that we might use to address the problem.

caesarsol commented 7 years ago

Importing slightly different rules from different versions of a single plugin seems theoretically logic but practically confusing, IMHO.

Wouldn't it be more straightforward to just fail-fast in a similar scenario, or maybe to make an arbitrary choice (plugin-wide or rule-wide) in case of version conflict?

ilyavolodin commented 7 years ago

After reading this thread, I think I actually agree with @caesarsol It seems like we are trying to preemptively fix an issue that isn't a real problem (at least, that's not something we've seen so far). While it's entirely possible to have very complex configuration with multiple configs inherited and deep nesting, it seems like it would be very rare, it also seems that the person that might go through the trouble of setting something like that up, knows what they are doing and should be able to fix it himself. Maybe we don't need to solve the issue of multiple versions of the same plugin?

ljharb commented 7 years ago

fwiw, Airbnb's use case for eslint-config-airbnb will be satisfied as long as:

nzakas commented 7 years ago

@ilyavolodin I disagree. How many times have we seen edge cases pop up from multiple people over a period of time that eventually forces us to make a change? I'm pretty exhausted by this discussion, so if we're going to solve this problem, then I want to solve it in such a way that we don't need to think about it ever again. :)

@not-an-aardvark

I'm not sure I understand this point. I thought a design goal here was that users should always be able to disable a given rule in their config.

They'd still be able to disable the rule, but the reference would be through the config rather than directly to the plugin.

nzakas commented 7 years ago

Okay, so I was lying awake in bed last night when I realized something: if we slightly change this discussion from "how can a shareable config bundle plugins as a dependency" to "how can a config bundle plugins as a dependency," then the problem is actually already solved because plugins can contain configs and can also specify other plugins as dependencies.

Example

Suppose Airbnb has a config they want people to use and that config depends on eslint-plugin-react. If the config is exposed in a plugin instead of a shareable config, you can make a plugin that looks like this:

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

// export the es6 config
module.exports.configs = {
    es6: require("./configs/es6.json")
};

// export the rules you want to expose
module.exports.rules = reactPlugin.rules;

The eslint-plugin-airbnb package can depend directly on eslint-plugin-react, so the dependencies are installed automatically with npm i eslint-plugin-airbnb. Then, in your local project config, you can do this:

plugins:
  - airbnb
extends:
  - plugin:airbnb/es6
rules:
  airbnb/jsx-uses-vars: "error"

In this case, the project config is extending the config from eslint-plugin-airbnb and the plugin is re-exporting the jsx-uses-vars rule from eslint-plugin-react, so you have to address it using airbnb/ instead of react/. IMHO, this is kind of nice because it makes the plugin dependency completely opaque. However, if you wanted a bit more transparency, you could change the plugin to prepend react/ to the beginning of each rule:

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

// export the es6 config
module.exports.configs = {
    es6: require("./configs/es6.json")
};

// export the rules you want to expose
module.exports.rules = {};
Object.keys(reactPlugin.rules).forEach(ruleName => {
    module.exports.rules["react/" + ruleName] = reactPlugin.rules[ruleName];
});

Then you could reference the rules as airbnb/react/jsx-uses-vars.

Evaluation

It seems like this approach solves all of the hard problems we've been trying to address with a decent amount of flexibility (in that plugin authors can decide which rules to export and how they should be named):

  1. Allows sharing of configs that depend on other packages without a separate install step.
  2. Allows multiple versions of the same plugin to be included by different plugins/configs without clashing.
  3. Gives plugin authors the ability to control how rules they depend on are represented in their plugins, so they can choose the best approach.
  4. Whether or not a config relies on a plugin is mostly opaque as far as the user is concerned.

Next Steps

As I've said before, shareable configs were really not designed to be used in the way we are trying to get them to be used in this thread, and I think the reason we keep banging into walls is because we're trying to force shareable configs into a paradigm they were never designed for.

Plugins, on the other hand, are made to be more composeable and looking back, it actually looked like we wanted to encourage people to use plugin configs instead of shareable configs when we first implemented it.

It seems like the best next step is for someone to actually try this out and give feedback on the experience. Any volunteers?

fson commented 7 years ago

@nzakas This sounds like a good solution and I'd be happy to give it a try in Create React App.

If it works, this would solve some persistent problems we've had:

I can try this out and give feedback on how it goes.

ljharb commented 7 years ago

That's a pretty huge breaking change for all the current users of eslint-config-airbnb - anything that actually solves this problem for me would need to work with users leaving "extends": "airbnb" as-is.

TheSavior commented 7 years ago

I agree with @ljharb here. Since eslint is very particular about the naming of packages and how plugins / configs are used, it's a pretty big problem that we have to migrate all of our users to a new package and deprecate the old one to solve this.

fson commented 7 years ago

Would it make sense to allow plugins to define a "default configuration" that would be used when the user has only a name of a plugin in extends (the way shareable configs can be used at the moment)? It seems that this way "extends": "airbnb" could continue to work with a new eslint-plugin-airbnb that would specify a default configuration.

ljharb commented 7 years ago

@fson i like that idea but there already exist conflicting eslint-config-foo and "eslint-plugin-foo with a default config" packages all over the place.

ilyavolodin commented 7 years ago

@ljharb I think it's still worth trying out first. If it proves to be working well with plugins, we could look into ways of either having ESLint handle configs as plugins, or allow ESLint to extend configs and plugins through the same mechanism. So far this is the first suggestion that we've had that seems like it might work, so I wouldn't want to dismiss it without trying it out first.

ljharb commented 7 years ago

@ilyavolodin there's certainly nothing wrong with improving plugins. I just want to make it clear that if it requires people to change their eslintrc or not be able to depend on the same eslint-config-airbnb, then it won't actually resolve the issue.

nzakas commented 7 years ago

@fson we actually used to define a default plugin config, but we decided it was too big of a surprise to users to have a plugin automatically enforce a config just because you included it. I don't think we want to go back there.

@ljharb sometimes improvements require breaking changes, unfortunately. It seems like in this case, the change from "airbnb" to "plugin:airbnb/foo" is easily solved with simple find-and-replace, as would be the change from "react/foo" to "airbnb/react/foo" or something else. So you could probably write a small utility to help people convert their existing configs if it proves to be a major pain point.

We can also help the cause by formally deprecating the eslint-config-* format and encouraging people to use plugins from now on (which it seems was my original thinking way back when).

ljharb commented 7 years ago

@nzakas If you accept that there's no way to make configs solve this problem, then of course it's required - but I don't think that's been proven in any way. If eslint wants to formally deprecate configs, then obviously my hand is forced (altho that's much better than informally deprecating them). (side note: i hope "extends": "plugin:airbnb" would be possible, so people wouldn't have to couple to a plugin's sub-config name)

I'm sure someone could write a utility to convert eslint-config-x into eslint-plugin-y, especially if a plugin can specify a default config (that required explicit extending ofc, not that was applied automatically), but that's a pretty big burden to impose on users.

nzakas commented 7 years ago

@ljharb I understand your point of view and concerns. I do believe it's been proven we can't do this in configs, and I don't think it's worth the time to continue down that path.

It would be really helpful if you (or anyone on this thread) could try to create a plugin as I described and see if it works for your use case. What we need now is feedback on what I described so we can move forward or not.

iamstarkov commented 7 years ago

@ljharb

I'm sure someone could write a utility to convert eslint-config-x into eslint-plugin-y, especially if a plugin can specify a default config (that required explicit extending ofc, not that was applied automatically), but that's a pretty big burden to impose on users.

prob its easy to write codemod for this

not-an-aardvark commented 7 years ago

I'm in agreement with a few other people on this thread that this would be a very unsatisfying solution.

we actually used to define a default plugin config, but we decided it was too big of a surprise to users to have a plugin automatically enforce a config just because you included it. I don't think we want to go back there.

I agree that it would be confusing for plugins to automatically enforce a config. But that's because enforcing a config by default is precisely the behavior that shareable configs are designed for. If we deprecate shareable configs and tell people to use plugins instead, then we need a good replacement for that behavior.


It feels like we're getting too caught up in the edge cases here, and it's causing us to throw out the idea of usable shareable configs entirely, even when it would make things easier in the vast majority of cases. The whole point of a shareable config is to be able to publish a style guide to npm, and allow users to enforce it without having to worry about configuration details. One of the design goals of ESLint is that users should be able to create custom rules so that core doesn't need to support everyone's requests. But as it currently stands, a shareable config publisher can only use ESLint's core rules, otherwise they incur an installation burden on all their users. For example, I can't use a rule from eslint-plugin-node in my shareable config without forcing users to install eslint-plugin-node manually. The fact that a particular rule comes from eslint-plugin-node, rather than core, is an implementation detail that that shareable configs are supposed to avoid.

This is enough of a problem that people use packages like standard, which is just a wrapper around ESLint that also has a preset config. Standard is popular because it makes it easy to just follow a styleguide, without worrying about configuration. If a beginner wants to download airbnb's style guide and enforce it on their code, they shouldn't have to worry about the distinction between plugins, shareable configs, and plugin-specific configs. They should be able to run npm install eslint-config-airbnb and put one extends: line, and have everything just work.

While I agree that we need to account for edge cases such as multiple versions of the same plugin, our solution shouldn't complicate enforcing a styleguide in general just because a shareable config decides to use a rule from a plugin instead of a core rule. Even implementing an inelegant solution for these edge cases would be better than forcing users to worry about the internals of a shareable config whenever it includes a plugin. While my proposal here has a lot of flaws (some of which might be fixable), I still think it would be a distinct improvement over the current behavior.

not-an-aardvark commented 7 years ago

Here's an updated proposal.

tl;dr:


Detailed version

Example

Suppose my project depends on eslint-config-airbnb, which uses rules from eslint-plugin-import. I could configure these rules the same way that I currently do:

  rules:
    import/no-unresolved: warn
  extends:
    - airbnb

Now I decide to also use eslint-config-node, which depends on a different version of eslint-plugin-import. So I install it and add it to my config file:

rules:
  import/no-unresolved: warn
extends:
  - airbnb
  - node

At this point, ESLint will throw an error, because I'm configuring the import/no-unresolved rule but it's unclear which version of eslint-plugin-import I'm referring to. To fix this, I can use a more specific reference:

rules:
  airbnb/import/no-unresolved: warn
extends:
  - airbnb
  - node

Note that I only need to change my own config file, and I don't need to change anything in eslint-config-airbnb. The reference to import in eslint-config-airbnb is still unambiguous, because eslint-plugin-node is not in eslint-plugin-import's dependency chain.

Disadvantages

One disadvantage to this proposal is that if it's implemented precisely as stated above, adding a plugin to a shareable config would be a breaking change, because it could cause a reference to become ambiguous. For example, suppose my config looks like this:

rules:
  import/no-unresolved: warn
extends:
  - airbnb
  - google

Since eslint-config-airbnb depends on eslint-plugin-import, but eslint-config-google does not, this reference is unambiguous, so the config is valid. However, if eslint-config-google adds eslint-plugin-import internally in a minor release, my config will break because the reference import/no-unresolved will become ambiguous.

I don't see this as a major concern, because if eslint-config-google starts depending on eslint-plugin-import, it's likely that the config turns on new rules from eslint-plugin-import, which would be a breaking change anyway.


Edit from a year later: I realized that this solution doesn't work well when using extends: with a relative path. Maybe there is a way to modify it to support that.

ljharb commented 7 years ago

That proposal sounds awesome and seems like it would solve my issue completely - as an added bonus, most people would just be able to trim down their package.json and wouldn't have to edit a single line of config!

ilyavolodin commented 7 years ago

@not-an-aardvark Your proposal address collisions with multiple configs/plugins of different version. It does not address the changes needed to allow configs to have hard dependency on plugins, however. How would you handle (with the same code) scenarios where plugin exposes shareable config and it's own rules and is extended by another shareable config? How would plugin with shareable config refer to it's own rules anyways?

not-an-aardvark commented 7 years ago

@ilyavolodin Thanks for the feedback on the proposal. However, I don't understand some of the issues you raised.

Your proposal address collisions with multiple configs/plugins of different version. It does not address the changes needed to allow configs to have hard dependency on plugins, however.

I'm not sure I understand the distinction you're making. As far as I can tell, the proposal would allow configs to have a hard dependency on plugins (that was the main design goal). Could you clarify why you don't think it would work?

How would you handle (with the same code) scenarios where plugin exposes shareable config and it's own rules and is extended by another shareable config? How would plugin with shareable config refer to it's own rules anyways?

This would work the same way as it currently does. The plugin's shareable config, and also a config that directly extends the plugin's shareable config, would both refer to the plugin's rules as plugin-name/rule-name.

mysticatea commented 7 years ago

Let me introduce an example of shareable configs which are using plugins. Somebody don't need to install the plugins to use the config.

This config depends on 3 plugins; eslint-plugin-eslint-comments, eslint-plugin-mysticatea, eslint-plugin-node. However, we can install the config with npm install --save-dev eslint eslint-config-mysticatea then we can use it. Users don't need to install the plugins manually.

In both cases, it would notify INVALID_PEER errors if it happened version conflicts of the plugins. So version conflicts of the plugins are notified as installation errors, we can act to resolve it.

I have been using this mechanism while 1.5 years and I have never encountered any problems. So honestly I wonder if this complex feature requires truly.

ljharb commented 7 years ago

@mysticatea relying on the automatic installation of peer deps, while also relying on npm 3+'s flattening, is certainly a very clever trick, but in no way do I think that's a proper solution to this problem. Additionally, because of the way your peer deps are defined, if I already have an existing top-level dep on eslint-plugin-node at, say, 2.x or 4.x - it will satisfy the peerDep requirement, but not satisfy the dependencies requirement - which will cause duplicates to be installed in my tree in npm 2, and the conflict will fail to hoist in npm 3 - which could either break, or silently do the wrong thing. It's fortunate that you've had no problems but this is a very brittle solution - one which is only lightly lessened if your peer+normal deps are pinned.

nzakas commented 7 years ago

If a reference to a plugin rule is ambiguous, ESLint throws an error and tells the user to be more specific. References to plugins are resolved such that the end user always has the ability to resolve an ambiguity.

I know this is a long thread, but I pointed out earlier that placing this burden on the end user is unfair and not something I support. It shouldn't be up to end users to understand a magical dependency path to get a rule to work -- that burden should be on the shareable config developer alone. Shareable configs should be black boxes to their consumers, anything more is going far beyond what is either expected or useful.

I'm still waiting to hear prototype feedback about using plugins instead. That seems to solve all of the problems on this thread without any changes. I'd like to ask again for some volunteer(s) to try out that approach and post feedback here.

I don't see any reason to keep coming up with other proposals until someone tries out the plugin approach to either prove or disprove it's viability as a solution.

ljharb commented 7 years ago

@nzakas i'm still unclear on how i can try out that approach without changes in eslint, since plugins that my plugin depends on still currently need to be top-level deps.

not-an-aardvark commented 7 years ago

@nzakas Thanks for the feedback.

It's unclear to me how shareable configs can be considered "black boxes". Even with their current behavior, the user has to know a lot about a config's internal dependencies and structure. For example, the user has to configure a rule based on the name of the plugin that it comes from (e.g. import/no-unresolved). Also, with the current behavior, the user has to manually install all of a shareable config's dependencies (although I realize that this is due to changes in npm and is not by design). If configs were really black boxes, the userule wouldn't have to worry about details like this. I think making the user consider a config's dependencies only in exceptional cases (i.e. when two versions are installed) is much better than making the user consider the dependencies every time they install a shareable config. As I mentioned above, it seems right now we're optimizing for very uncommon edge cases while making the most common use-case significantly less convenient.

ilyavolodin commented 7 years ago

@nzakas The problem I have with your approach, is it sort of promotes marketing somebody else's work as your own. While in your example you export rules out as react/ruleName, not everyone might do that since it's not enforced in any way, in that case, to the end user it will look like rules that are configured by current plugin are also written by the same person. This makes for ambiguous ownership as well as might make it harder to report bugs with the rule in appropriate repository.

maxnordlund commented 7 years ago

This bite today, and after reading through this thread I think it would be best if eslint itself shipped any sort of upgrade tool, like babel-doctor/brew doctor, that could handle and changes in syntax. E.g. extends: "airbnb" -> extends: "plugin:airbnb", and since this would be a one-time thing I do think it's OK to force upon end users.

One disadvantage of this is how to know what the equivalent plugin would, and for that the config needs to somehow specify that. That could probably go into the package.json, or exported as a special property.

But what to do with legacy configs? First off, the same doctor command could warn and suggest/fix sharable configs, but unless you go with some sort of rules like @not-an-aardvark suggests. Alternatively, or perhaps complementary, is to analyze the peer dependencies for the end user and sync it with their packages.json. This would allow any config install to be reduce to npm install --save eslint-config-foo; eslint sync/doctor/whatever, or even eslint install --config foo.

Not sure if I like the latter though, but such an approach would "solve" the issue today from the eyes of an end user, and allow smooth upgrading when a good solution exists. (Stripping out the peer dependencies and update the user config).

I'm sure there's many things I missed, but the important point is to ship an official doctor/upgrade command when the time comes.

mmazzarolo commented 7 years ago

Shameless plug. If you need an example I used the hints in this thread to create an eslint-plugin with bundled dependencies: eslint-plugin-react-app.

ljharb commented 7 years ago

@mmazzarolo What happens if there's two conflicting copies of one of your plugin dependencies in the tree?

mmazzarolo commented 7 years ago

@ljharb I'll be honest: I don't know, I haven't played with them yet.
I have been using this lib for a while on multiple projects without any issue, but I'm almost sure that my strategy would not suit at all your use case.

not-an-aardvark commented 6 years ago

I was reading through this thread again, and I noticed a point of confusion:

@nzakas i'm still unclear on how i can try out that approach without changes in eslint, since plugins that my plugin depends on still currently need to be top-level deps.

I don't think @nzakas's proposal would require any changes to ESLint itself. In that proposal, people would share their configs using plugins, and the plugins could internally call require on other plugins and export their rules. For example, one could create eslint-plugin-airbnb containing code like this:

const importPlugin = require('eslint-plugin-import');

module.exports.rules = {
  'no-unresolved': importPlugin.rules['no-unresolved'],
  // ...
};

ESLint wouldn't care that eslint-plugin-airbnb got the rule from eslint-plugin-import -- as far as ESLint is concerned, the rule would be airbnb/no-unresolved. This would mean that eslint-plugin-airbnb could have eslint-plugin-import as a dependency, not a peerDependency.

There are still a few downsides of this proposal, (described here and here), but I wanted to clarify that it does not require any changes to eslint to work.

ljharb commented 6 years ago

Thanks for clarifying.

Indeed, those downsides means that I could not ship a package that worked in this way without breaking a large number of users eventually.

ianrtracey commented 6 years ago

+1 on this issue. Would love to have it as well.

thibaudcolas commented 6 years ago

I've attempted the "plugin exposing config and specifying other plugins as dependencies" approach highlighted above by @nzakas, following @mmazzarolo's eslint-plugin-react-app example.

It works, but I found another problem/downside: if the config exposed by the plugin extends other configs, the plugin needs to rewrite the configured plugin rules in all of this tree of extends to prefix them with its name.

Without this traversing/prefixing, the linting would show errors coming from the bundled plugins directly. Oh and I think this goes beyond rules – since we can't rely on extends (rules would not be prefixed), then all of the configuration may need to be recursively merged together during the traversal.

My ESLint knowledge only goes so far so please let me know if I've missed something. Here is my real-world example, and (hacky) solution to this problem, relying on ESLint's CLIEngine#getConfigForFile to compute the final config before prefixing the rules.

Edit: extra food for thought – while this approach technically works without relying on brittle transitive dependencies, I find the idea of prefixing all plugin rules with my config (company's) name quite ugly. This makes configs and inline overrides slightly harder to reuse beyond our projects, and slightly harder to search for.

pzuraq commented 6 years ago

Trying to put together a shared config and running into these issues as well. I tried to go down the plugin approach but agree that it feels hack-ish, especially when you consider that you have to walk the entire tree as @thibaudcolas described.

I like @not-an-aardvark's proposal, it makes sense in most cases and like he points out adding plugins in a config will likely force a breaking change anyways, so it's ok to make it always happen. Alternatively, we could disallow plugin resolution from a config altogether. If you want to override a rule provided by a plugin, you would have to install the plugin at the level you're trying to override it at.