Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support having plugins as dependencies in shareable config #3458

Open
sindresorhus opened this issue Aug 20, 2015 · 182 comments
Open

Support having plugins as dependencies in shareable config #3458

sindresorhus opened this issue Aug 20, 2015 · 182 comments

Comments

@sindresorhus
Copy link
Contributor

@sindresorhus sindresorhus commented Aug 20, 2015

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.

@eslintbot
Copy link

@eslintbot eslintbot commented Aug 20, 2015

Thanks for the issue! We get a lot of issues, so this message is automatically posted to each one to help you check that you've included all of the information we need to help you.

Reporting a bug? Please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. The source code that caused the problem
  3. The configuration you're using (for the rule or your entire config file)
  4. The actual ESLint output complete with line numbers

Requesting a new rule? Please be sure to include:

  1. The use case for the rule - what is it trying to prevent or flag?
  2. Whether the rule is trying to prevent an error or is purely stylistic
  3. Why you believe this rule is generic enough to be included

Requesting a feature? Please be sure to include:

  1. The problem you want to solve (don't mention the solution)
  2. Your take on the correct solution to problem

Including this information in your issue helps us to triage it and get you a response as quickly as possible.

Thanks!

@eslintbot eslintbot added the triage label Aug 20, 2015
@lo1tuma
Copy link
Member

@lo1tuma lo1tuma commented Aug 20, 2015

We already discussed this in #2518 with the conclusion that a
peerDependency is the way to go.

@sindresorhus
Copy link
Contributor Author

@sindresorhus sindresorhus commented Aug 20, 2015

Ugh, that's such a leaky abstraction. I guess I won't use plugins then...

The user shouldn't have to care what plugins I use for the rules. This is like requiring to manual install of Lodash when you want ESLint. A shareable config is a node module and should act like it.

@lo1tuma
Copy link
Member

@lo1tuma lo1tuma commented Aug 20, 2015

A shareable config is a node module and should act like it.

We use require to load shareable configs or plugins, what would make it act more like a node module than that?

This issue should be also solved when using npm version 3 which installs all subdependencies in the top-level node_modules folder.

@sindresorhus
Copy link
Contributor Author

@sindresorhus sindresorhus commented Aug 20, 2015

We use require to load shareable configs or plugins, what would make it act more like a node module than that?

Let the shareable config provide the plugin as an object:

module.exports = {
    plugins: [
        require('eslint-plugin-no-use-extend-native')
    ],
    env: {
        node: true
    },
    rules: {
        'comma-dangle': [2, 'never'],
        'no-cond-assign': 2
};

This issue should be also solved when using npm version 3 which installs all sub-dependencies in the top-level node_modules folder.

That's an implementation detail and not always guaranteed. Nobody should ever depend on that. npm@3 promises flatter dependency tree, not flat. If there are conflicts, there will be nesting.

@feross
Copy link
Contributor

@feross feross commented Aug 20, 2015

The option to require the plugin in the config itself would allow users to use my shareable config without needing to manually install two other plugins.

I like this proposal.

@lo1tuma
Copy link
Member

@lo1tuma lo1tuma commented Aug 20, 2015

@sindresorhus good point about npm 3, let's forget about this.

I kind of like your proposal, but it has a few problems:

  1. Eslint needs to know the name of the plugin. This could be solved easily by providing an object with the name e.g plugins: [ { 'eslint-plugin-foo' : require('eslint-plugin-foo')} ]
  2. Eslint caches plugins once they are loaded. This could be a problem if you use different shareable configs in different .eslintrc files where the shareable configs require the same plugin, but in a different version. Possible solution would be to avoid caching in this case.
@BYK
Copy link
Member

@BYK BYK commented Aug 20, 2015

Possible solution would be to avoid caching in this case.

Or we can prefix plugins provided by shareable configs with the name of the config?

@lo1tuma
Copy link
Member

@lo1tuma lo1tuma commented Aug 20, 2015

@BYK how would you reference the rules then? configname/pluginname/rulename? But I guess we would have the same problem if we avoid caching. We can't determine to which version of the plugin the rule belongs to.

That said, I think we should first decide if we want this feature in ESLint.

@BYK
Copy link
Member

@BYK BYK commented Aug 20, 2015

@BYK how would you reference the rules then? configname/pluginname/rulename?

Yep.

That said, I think we should first decide if we want this feature in ESLint.

Agreed. Might be worth piggy backing on npm 3 instead of introducing this complexity.

@nzakas
Copy link
Member

@nzakas nzakas commented Aug 20, 2015

A few things:

  1. npm 3 doesn't solve this problem, the relationship between a config and a plugin remains a peer relationship. The fact that npm 3 flattens dependencies doesn't fundamentally change that relationship, is just an implementation quirk that allows dependencies to be treated as peers in certain situations. That's not a solution, is a gamble.
  2. Configs should not contain executable code, that's very far outside of the responsibilities of configs.
  3. To keep this in context: we are talking about a one-time setup cost rather than ongoing pain.

While I can understand the desire to have one install that works, I don't see a path towards that without introducing a new type of shareable thing that could encapsulate this functionality.

@sindresorhus
Copy link
Contributor Author

@sindresorhus sindresorhus commented Aug 21, 2015

Then maybe introduce a universal sharing thing that can contain multiple plugins/configs/whatever. It could even in the future allow extending ESLint in some ways, with hooks, but I don't want to start that discussion. Just showing the possibilities with something like this.

JSCS supports it like this: https://github.com/wealthfront/javascript/blob/f1f976e9c75a8d141ec77a5493d9d965d951d4a6/jscs/index.js

I just want the user to be able to npm install one module and have the needed config and plugins without having to care about how anything works internally. That's the beauty of normal npm packages.

@IanVS
Copy link
Member

@IanVS IanVS commented Aug 21, 2015

I agree that the current method becomes unwieldy when you begin sharing configs which use other shared configs and/or plugins. For example, the installation instructions for my own personal config (which extends from Standard) is:

npm install --save-dev eslint-plugin-standard eslint-config-standard eslint-config-ianvs 

It would be much nicer UX to only need:

npm install --save-dev eslint-config-ianvs 

That said, I have no idea how that could be accomplished, and in the end it's a pain I can live with until/unless a better solution is found.

@nzakas
Copy link
Member

@nzakas nzakas commented Aug 24, 2015

We could extend plugins to allow the inclusion of configs, as plugins were always intended to be a dumping ground of stuff. Thoughts:

  1. How do we specify them in extends? eslint-plugin-foo.configs.whatever? Something else?
  2. We could, in theory, just expose the file system so you could do eslint.plugin-foo/configs/whatever.
  3. This is a bit lousy because we lose the nice eslint-config-* convention for configurations, so it ends up blurring what is a configuration and what is not.
  4. What if a config wants to depend on a plugin that it doesn't own? What does that look like?
  5. What is the expected behavior when a plugin is install directly and the same plugin is installed indirectly via another plugin?
@feross
Copy link
Contributor

@feross feross commented Aug 30, 2015

@nzakas

Configs should not contain executable code, that's very far outside of the responsibilities of configs.

Could you elaborate on this? It seems like this is a philosophical rather than practical objection. From a user's perspective, an eslint config is just an npm package that they need to install and extend in their .eslintrc. They don't care if there's executable code in there or not. Why complicate things for users?

@nzakas
Copy link
Member

@nzakas nzakas commented Aug 31, 2015

@feross Allowing executable objects arbitrarily in configs would complicate things for users. What I'm saying is let's not complicate it by ensuring that configs remain static regardless of their form.

@joakimbeng
Copy link
Contributor

@joakimbeng joakimbeng commented Sep 1, 2015

Let the shareable config provide the plugin as an object

👍 would love to have this functionality!

We use require to load shareable configs or plugins, what would make it act more like a node module than that?

The problem is that the plugin name is not left as is, but instead parsed and prepended with eslint-plugin-. If ESLint didn't do this one could have solved the problem by adding plugins by their full paths, e.g. plugins: [path.join(__dirname, 'node_modules', 'eslint-plugin-babel')], not fancy but it would probably work.

@nzakas
Copy link
Member

@nzakas nzakas commented Sep 1, 2015

We don't have a good answer for this now. We'll revisit once we've finished some 2.0.0 tasks.

@ilyavolodin
Copy link
Member

@ilyavolodin ilyavolodin commented Sep 16, 2015

Related to #3659

@davidmason
Copy link

@davidmason davidmason commented Oct 20, 2015

It seems as though this is the case for configs as well, unless I am mistaken. For configs at least, is it possible to change how extends are loaded so that nested extends are processed in the module context that they come from?

This could at least solve the issue for configs, which do not have the issue of executable code.

e.g.

  • say I am making a shareable config for my team's projects, eslint-config-myteam
  • I want to base myteam config on another shareable config eslint-config-goodstyles with a few modifications.
  • in one of my team's projects ("myproject"), I npm install eslint eslint-config-myteam and create .eslintrc that extends: myteam
// eslint-config-myteam/index.js
module.exports = {
  extends: 'goodstyles'
}
# myproject/.eslintrc
extends: myteam

So when eslint is processing myproject/.eslintrc and finds extends: myteam it will locate node_modules/eslint-config-myteam.

At the moment I think it blindly reads that in, then fails when it hits the nested extends: goodstyles because that is not available at the top level. Could it instead keep track of which module it found the myteam config in, and if it finds an extends in there, search in that module for the config it extends. There are a few options for how to search:

  1. look only in the specific module that the extending config is from
  2. look first in the specific module that the extending config is from, then look at the higher level if it is not found there
  3. look first in the module where eslint was run, then in the specific module if the config is missing form there

The question is whether people should be able to override configs by name (on purpose or otherwise) in their extending config. Overriding configs by accident would be possible with 3, so I would rule that out. 1 would not allow peer-dependency configs, so I think 2 is the best option - if someone wants to make other configs peer dependencies they can just not include them in their package.json, but there is the option to include them and make life easier for consumers of their shared config.

@kirill-konshin
Copy link

@kirill-konshin kirill-konshin commented May 12, 2020

I've got a question regarding 7.0.0 release notes: https://eslint.org/blog/2020/02/whats-coming-in-eslint-7.0.0#plugins-loaded-from-config-file-directory

In v7.0.0, plugins will be loaded relative to the configs that reference them. You can read more in the RFC.

This is a confusing statement. Seems that it means just the root config, not the ones that it extends.

Are there any plans to fix this issue?

@vjpr
Copy link

@vjpr vjpr commented Jun 2, 2020

If you are working in a monorepo, your natural instinct is to bundle all your plugins, extends, and rules into a single dependency to declutter your dependencies, and share this amongst all your packages. But it seems this is not possible.

I would assume that most people would come to eslint thinking that the config would behave the same as babel where you can do require.resolve('plugin') or require('plugin').

It might be worth adding a note to the docs to make it clear that it is not possible to pass filenames to extends like in babel. There is an error message that detects filenames, but you will get some other wierd errors if you pass a module.

Workaround

WebStorm + CLI

ESLint > Extra ESLint options = --resolve-plugins-relative-to=<full-path-to-shareable-config>

EDIT: WebStorm not working. See issue.

Create a wrapper script or add to npm scripts: eslint --resolve-plugins-relative-to=<full-path-to-shareable-config>

@octogonz
Copy link

@octogonz octogonz commented Jun 3, 2020

If you are working in a monorepo, your natural instinct is to bundle all your plugins, extends, and rules into a single dependency to declutter your dependencies, and share this amongst all your packages. But it seems this is not possible.

@vjpr I solved it using the monkey patch described in this comment. It patches ESLint's module resolution to work the way you would expect.

I later opened PR #12460 to propose integrating this feature into ESLint, however that PR got hamstrung by the RFC process.

I gave up because the monkey patch works perfectly for our needs:

  • patch-eslint6.js - how the patch is used by a shared toolchain in a production monorepo
  • .eslintrc.js - an example monorepo project that loads this patch so that its dependencies will resolve correctly

If people show interest in merging the PR, I would consider revisting it. But the general impression was that the ESLint maintainers prefer to completely revamp the module loader architecture versus accepting a minimal (and opt-in) workaround for the existing architecture. The concern was that it might be difficult to support.

@vjpr
Copy link

@vjpr vjpr commented Jun 3, 2020

@octogonz The workaround I posted works is currently working for me. Need to patch IntelliJ though because its missing one line config option here.

@octogonz
Copy link

@octogonz octogonz commented Jun 3, 2020

The workaround I posted works is currently working for me. Need to patch IntelliJ though because its missing one line config option here.

I did consider --resolve-plugins-relative-to but for our case it had a couple issues:

  • It can only resolve relative to one thing -- what if you have more than one shared package?
  • CLI parameters aren't discoverable by ESLint tools such as the VS Code add-in. Even if the add-in provides some setting that can modify the CLI, it's going to be a proprietary feature that is different for each editor.
@danez
Copy link

@danez danez commented Jun 16, 2020

For me this now got even worse with eslint v7.

We are using spire to run eslint in our projects. How it works is simple: in a project I only install spire and a spire-config. The config depends on eslint, eslint-configs and eslint-plugins. The configs we use have a hierarchy, for example typescript-node-config extends nodejs-config which extends base-config. Each of the configs might use other plugins which they peerDepend on. The spire-config in the end begins everything together by fulfilling these peerdeps.

When using eslint v7 the plugins are not found anymore, because yarn is not hoisting them anymore to the top. Not sure why though.

Imho instead of resolving the plugins from the entry-config it should be resolved from the config file that mentions/uses them.
So for example /project/.eslintrc.js extends /other/config.js which uses a plugin, then the plugin should be resolved from /other/node_modules. Maybe if people have the need for the current way there could be a fallback to /project/node_modules if not found in the other location.

@adidahiya
Copy link

@adidahiya adidahiya commented Jun 16, 2020

I have a use case similar to @danez (not with spire, but a custom build infrastructure) where we wrap eslint in a CLI tool which configures a base configuration bundled in the same NPM package as the CLI. The monkey-patch we had working with eslint v6 now has to be updated after the breaks in v7.

As others have briefly mentioned above, I believe this situation gets worse with package managers which don't hoist like yarn, such as pnpm. So I hope eslint/rfcs#9 does support isolating plugin dependencies to the configs where they are enabled, and that change is rolled out soon before further breaks to ESLint config resolution.

@kirill-konshin
Copy link

@kirill-konshin kirill-konshin commented Jun 16, 2020

Here's the moneky-patch for v7:

#!/usr/bin/env node
/* eslint-disable @typescript-eslint/no-var-requires */

const fs = require('fs');

const patchPaths = [
    // require.resolve('eslint/lib/cli-engine/cascading-config-array-factory'),
    require.resolve('eslint/lib/cli-engine/config-array-factory')
];

const pkg = require('../../package.json');

const token = 'internalSlotsMap.set(this, {';
const addon = `resolvePluginsRelativeTo = require.resolve('${pkg.name}');\n        ${token}`;

patchPaths.forEach(path => fs.writeFileSync(path, fs.readFileSync(path).toString().replace(token, addon)));

https://github.com/ringcentral/ringcentral-javascript/blob/master/eslint-config-ringcentral-typescript/src/bin/rc-eslint-patch.js

We call it as bin script of our shared config.

@adidahiya
Copy link

@adidahiya adidahiya commented Jun 16, 2020

Here is our version of the monkey-patch for eslint v7 (adapted from @octogonz #3458 (comment) above):

/**
 * Definition from eslint/lib/cli-engine/config-array-factory
 */
interface ConfigArrayFactoryLoadingContext {
    /** The path to the current configuration. */
    filePath: string;
    /** The base path to resolve relative paths in `overrides[].files`, `overrides[].excludedFiles`, and `ignorePatterns`. */
    matchBasePath: string;
    /** The name of the current configuration. */
    name: string;
    /** The base path to resolve plugins. */
    pluginBasePath: string;
    /** The type of the current configuration. This is `"config"` in normal. This is `"ignore"` if it came from `.eslintignore`. This is `"implicit-processor"` if it came from legacy file-extension processors. */
    type: "config" | "ignore" | "implicit-processor";
}

export function dangerouslyPatchESLint() {
    const ModuleResolver = require("eslint/lib/shared/relative-module-resolver");
    const { ConfigArrayFactory } = require("eslint/lib/cli-engine/config-array-factory");
    const originalLoadPlugin = ConfigArrayFactory.prototype._loadPlugin;
    ConfigArrayFactory.prototype._loadPlugin = function (_name: string, ctx: ConfigArrayFactoryLoadingContext) {
        const originalResolve = ModuleResolver.resolve;
        try {
            // Resolve using current config filePath instead of `relativeToPath`
            ModuleResolver.resolve = function (moduleName: string, _relativeToPath: string) {
                return originalResolve.call(this, moduleName, ctx.filePath);
            };
            return originalLoadPlugin.apply(this, arguments);
        } finally {
            ModuleResolver.resolve = originalResolve;
        }
    };
}
@octogonz
Copy link

@octogonz octogonz commented Jun 24, 2020

Update: Since people found it useful, we've released our patch as a standalone NPM package: @rushstack/eslint-patch

I updated the implementation to work with both ESLint 6.x and 7.x. We'll maintain it until whenever ESLint can provide an official solution. Thanks all!

@nzakas
Copy link
Member

@nzakas nzakas commented Aug 8, 2020

An update on this from ESLint team: The official way we will be supporting plugins as dependencies in shareable configs is through the new simple config system (eslint/rfcs#9). You can follow the implementation progress here:
#13481

zappen999 added a commit to mybonus-com/eslint-config that referenced this issue Sep 28, 2020
Using '@rushstack/eslint-patch' in order for eslint to utilize
dependencies in this package, without having the package-consumer
install all dependencies manually. More info:

eslint/eslint#3458

Note that 'eslint-plugin-prettier' will run prettier as a plugin, so the
developer should not need to run prettier manually.
bryanbraun added a commit to sparkbox/eslint-config-sparkbox that referenced this issue Oct 20, 2020
…to dependencies"

This reverts commit ab37b7f.

Reverting this because it turns out that we can't actually declare
eslint plugins as dependencies. There's a feature request for this
that has been open since 2015: eslint/eslint#3458

This is also described in the docs but I missed it because I mixed
up "sharable configs" with "plugins". More details on that here:
https://eslint.org/docs/developer-guide/shareable-configs#publishing-a-shareable-config

It looks like there is a "simple config" initiative in progress which
should improve this (eslint/eslint#13481).
But until that's available, it's probably best to revert this to the
existing approach.
@bodograumann
Copy link

@bodograumann bodograumann commented Oct 23, 2020

I was under the impression, that this problem was finally fixed in v7. According to the announcement:

ESLint will now resolve plugins relative to the entry configuration file. This means that shared configuration files that are located outside the project can now be colocated with the plugins they require.

Is it just me misunderstanding or is the announcement misleading?

@mrmckeb
Copy link
Contributor

@mrmckeb mrmckeb commented Oct 23, 2020

I think the announcement was not misleading intentionally, but definitely confusing. I also initially read it and thought "great news", and then saw that the related issues were still open.

@milichev
Copy link

@milichev milichev commented Nov 4, 2020

@octogonz , thank you for your @rushstack/eslint-patch!
Unfortunately, it didn't solve the problem with my very simple configuration:

  • yarn 2 (berry just built from master)
  • react-scripts@4.0.0
  • eslint@^7.11.0 (dependency of react-script)
  • moved eslint config from package.json to .eslintrc.js and added require("@rushstack/eslint-patch/modern-module-resolution");

Still, react-scripts start fails with "Failed to load config "react-app" to extend from"
Therefore I decided to rollback to yarn 1.22 for now.

I'll appreciate any assistance in solving the issue.

@octogonz
Copy link

@octogonz octogonz commented Nov 5, 2020

@milichev Please open a Rush Stack issue and include some repro steps. That's where we are providing support for @rushstack/eslint-patch. Thanks!

@volodymyr-kushnir
Copy link

@volodymyr-kushnir volodymyr-kushnir commented Nov 5, 2020

@milichev you may also want to take a look at this thread here facebook/create-react-app#9446. Your setup is very similar to the one I'm using (except for the eslint-patch — I don't use that, I just follow this thread for other reasons) and with a little workaround everything works for me now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

You can’t perform that action at this time.