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

Declaring shareable configs and plugins in package.json is unreliable #10125

Closed
not-an-aardvark opened this issue Mar 25, 2018 · 16 comments

Comments

Projects
9 participants
@not-an-aardvark
Copy link
Member

commented Mar 25, 2018

As of ESLint v4.19.1, all shareable configs and plugins are loaded from the location of the eslint package. This behavior is clearly documented, and we advise users that if they want their config/plugin packages to be loaded, they should ensure that the packages are placed where eslint can find them.

However, there is a problem with this approach: package managers provide no way for users to ensure that the packages will be placed in a valid way. There are a few common practices that people use to load their configs/plugins, but all of them are actually relying on an implementation detail of a package manager rather than a well-defined behavior.

For example, in order to use a config that depends on "eslint-config-foo" and "eslint-plugin-bar", we recommend putting this in a package.json file:

{
  "devDependencies": {
    "eslint": "^4.19.2",
    "eslint-config-foo": "1.0.0",
    "eslint-plugin-bar": "1.0.0"
  }
}

This package.json file imposes a requirement on a package manager: the eslint, eslint-config-foo, and eslint-config-bar packages must be accessible from the root package. However, it does not impose any requirement that the packages are accessible from each other, which is actually what the user needs.

It happens to be the case that with the most common package management strategy, the packages are accessible from each other anyway, because the packages are laid out like this:

(my project root)/
├── .eslintrc.json
└── node_modules/
    ├── eslint/
    └── eslint-config-foo/
    └── eslint-plugin-bar/

But I want to stress that this is an implementation detail of package managers, and not a required behavior. As a result, some package management strategies arrange packages in a way that breaks ESLint. For example, lerna would sometimes arrange packages like this:

(monorepo root)/
├── node_modules/
|   ├── eslint/
|   └── eslint-plugin-bar/
└── packages/
    ├── eslint-config-foo/
    |   └── index.js
    └── (my project root)/
        ├── .eslintrc.json
        ├── package.json
        └── node_modules/
            └── eslint-config-foo (symlink to (monorepo root)/packages/eslint-config-foo)

In this example, lerna is fulfilling all of the requirements in package.json: eslint, eslint-config-foo, and eslint-plugin-bar are all reachable from (my project root)/. However, ESLint fails to run because eslint-config-foo is not reachable from eslint.

It should be noted that adding a peerDependency of eslint in a config/plugin doesn't help in this case. If eslint-config-foo has a peerDependency of eslint, this imposes a requirement that the same version of eslint is reachable from both the project root and from eslint-config-foo. However, there is no requrement that eslint-config-foo is reachable from eslint. In the package layout above, all peerDependencies would be satisfied, but eslint would still be broken.

Similarly, we recommend that shareable configs include all of their plugins as peerDependencies, but this is also unreliable, because it only imposes a requirement that the plugin is reachable from the config and the project root. It doesn't impose a requirement that the plugin is reachable from the eslint package.


To summarize the problem, config file loading is generally fragile when using a package manager, because eslint has an unusual requirement about package layout and the user has no way to ensure that the requirement is satisfied. As a result, a lot of people end up getting confused about why eslint can't find a package, and while there are solutions that appear to work in most cases, the solutions break down when the user has an uncommon-but-valid package management workflow.

I think the current plugin/config-loading system was designed when npm (specifically npm<=2) was the only prevalent package manager that people used for Node packages. As a result, the current system encourages a pattern with peerDependencies that happened to work well when using npm2, but actually relied on implementation details rather than a well-defined behavior. Now that it's common for people to use other package management strategies (such as lerna and yarn workspaces), I think it's important that we upgrade to be compliant with how packages are supposed to work, so that users can declare their dependencies robustly.

When discussing this before, we've described it as a distinction between "local" and "global" installations, and we encourage users to install all packages "locally". However, I think this oversimplifies the issue because it assumes that everything will work fine if packages are installed locally. In fact, all of the packages in the examples above have packages installed "locally", but they still can cause ESLint to break.

After going back and forth a few times, this problem has convinced me that we should fix the "global versus local" issue by resolving all config and plugin names from the location of the config file where they appear. This would make ESLint's behavior consistent with how modules generally work in Node when calling require -- the package that contains the module name is in charge of providing it as a dependency. This behavior also has other benefits of a well-designed module system (e.g. it's possible to have nested dependencies, so modules are encapsulated and there isn't a global namespace of module names).


I think the one major blocker for this change is that ESLint can't currently handle loading multiple plugins with the same name, because users need to be able to uniquely refer to a plugin when they configure its rules. With the current system, this can't occur because plugins need to be reachable from eslint in node_modules, which effectively guarantees a namespace without conflicts. (Unfortunately, this method of getting unique plugin names has significant downsides, e.g. it's not possible to load a plugin from a path because it could conflict with a name from node_modules.) This issue has also blocked #3458 and #6237.

To resolve it, I think we should investigate a solution involving plugin namespaces, (along the lines of #3458 (comment)), where users can more precisely define a rule in a config if necessary, in order to avoid naming conflicts. In my opinion, this would be a much better solution than dumping plugins into a global namespace and making the user avoid conflicts at installation time.

@arcanis

This comment has been minimized.

Copy link

commented Sep 27, 2018

We've started to get reports warning of incompatibilities with ESLint: facebook/create-react-app#5117

Is there a not-perfect-but-good-enough-in-practice solution we can recommend to our users? From the top of my head, these are the possible solutions, and the reason they don't currently work:

  • We could recommend them to require the plugins themselves. This doesn't work because ESLint requires the plugins to be plugin names.
  • We could recommend them to use require.resolve to give ESLint an explicit path to the module. This doesn't work because ESLint automatically prepends eslint-plugin at the beginning of the absolute path, making it invalid.

Could you consider changing the second point so that /foo/bar will become /foo/eslint-plugin-bar instead of eslint-plugin-/foo/bar? And /foo/eslint-plugin-bar would stay unchanged? I'm open to make the PR myself if I get the greenlight. It won't solve the duplicate package problem, but at least give a workaround until #10643 gets implemented 🙂

cc @ilyavolodin @gaearon

@not-an-aardvark

This comment has been minimized.

Copy link
Member Author

commented Sep 27, 2018

TSC Summary: ESLint's plugin- and config-loading mechanism currently depends on implementation details of package managers like npm and yarn, rather than intended behavior. (In short, eslint needs to be able to load plugins like eslint-plugin-react relative to its own location, even though eslint never specifies eslint-plugin-react in its own dependencies.) This causes ESLint to unexpectedly break when using package management tools like Lerna or Yarn Plug N' Play, which is creating problems for downstream tools.

It appears that any solution to this problem would require shareable configs to be able to load their plugins from their own dependencies (or from a config-provided absolute path). As discussed in #3458 and #10643, this would introduce a problem if two loaded plugins have the same name, because the user wouldn't be able to independently configure the rules from the two plugins.

It seems like we have several ways we could proceed:

  1. Load plugins from the dependencies of the config that specifies them, and introduce a mechanism for users to disambiguate plugins with the same name (e.g. as proposed in #10643). Note that #10643 is semver-major.
  2. Load plugins from the dependencies of the config that specifies them, and throw a fatal error if two loaded plugins have the same name. (This would also be semver-major, although it would require significantly less implementation effort than (1).)
  3. Continue loading plugins from the location of the eslint package, but allow configs to specify absolute paths to plugins. Throw a fatal error if two loaded plugins have the same name. (This would not be semver-major, although it would introduce the same potential user confusion (2).
  4. Don't change anything, and decide that we don't support using Lerna or Yarn Plug N' Play. I would not recommend this option; ostensibly a linter shouldn't care what package management strategy is being used.

Solution (3) seems to be the only option that fixes the issue without a breaking change. If we adopt solution (3), I think we should commit to also adopting solution (1) or (2) in the next major release, otherwise the situation will end up very messy. In past discussions, the idea of throwing a fatal error when encountering duplicate plugin names (i.e. implementing (2) or (3)) has been very contentious.

I think solution (1) would be the best solution in the long term, but based on past discussions I suspect this opinion might also be contentious.

TSC Question: How should we address this issue?

@not-an-aardvark

This comment has been minimized.

Copy link
Member Author

commented Sep 28, 2018

This issue was discussed in today's TSC meeting. The resolution was that we plan to solve this issue with option (1) from #10125 (comment) in a major release. (The solution would probably be #10643, although it's still pending an implementation.) We decided not to add a stopgap solution like option (3) in the meantime, because it might have the potential to create confusion after (1) is implemented, and it might interfere with the solution for (1) if the proposal changes.

@sibelius

This comment has been minimized.

Copy link

commented Oct 31, 2018

I think we need a guide to how to setup eslint when using yarn workspaces, lerna and monorepos structures

we created this monorepo boilerplate https://github.com/entria/entria-fullstack to experiment with monorepos and be easy to reproduce some issues we are having

@evocateur

This comment has been minimized.

Copy link

commented Oct 31, 2018

@sibelius

This comment has been minimized.

Copy link

commented Oct 31, 2018

what if we want to have different eslint configs per package?

imagine that I have server, web and app

web should have eslint-plugin-react
app should have eslint-plugin-react-native

and so on

the problem is that having a .eslintrc.js in each package, the eslint won't find eslint plugins because they will be hoisted to root

@ljharb

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

I don’t see why they wouldn’t be found; require looks recursively upwards

@evocateur

This comment has been minimized.

Copy link

commented Oct 31, 2018

@sibelius

This comment has been minimized.

Copy link

commented Oct 31, 2018

moving to root works great, tks

the problem is when trying to use together with jest-runner-lint (https://github.com/jest-community/jest-runner-eslint)

as it makes you have one config for each package

@ljharb

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

@sibelius you can configure all of those at the root, too, using jest's "projects" feature.

@not-an-aardvark

This comment has been minimized.

Copy link
Member Author

commented Oct 31, 2018

Hi everyone, can we move the discussion about how to set ESLint up to the Gitter channel or a new issue? It doesn't really seem related to this issue.

@medikoo

This comment has been minimized.

Copy link

commented Nov 5, 2018

@not-an-aardvark solution as described at #10643 looks great.

The only usability concern I see (referring to an example from #10643), is that in scenario when both eslint-config-foo and eslint-config-baz depend on same version of eslint-plugin-react,
to tweak some rule from eslint-plugin-react, we will be expected configure our tweaks twice, separately for foo::react and bar::baz::react.

Wouldn't it be better to still accept no prefixes form (so react) in such case, and throw only if given config tweak seems not compatible with used plugin version?

@nzakas

This comment has been minimized.

Copy link
Member

commented Nov 19, 2018

The ESLint team has just created an RFC process for complicated changes that require designs. This issue was marked as "needs design" and so falls into this new RFC process. You can read more about the RFC process here:

https://github.com/eslint/rfcs/

Thanks!

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Nov 22, 2018

yarn: Update to 1.12.3.
Changelog tracks back up to 1.12.0 only.

## 1.12.3

**Important:** This release contains a cache bump. It will cause the very first install following the upgrade to take slightly more time, especially if you don't use the [Offline Mirror](https://yarnpkg.com/blog/2016/11/24/offline-mirror/) feature. After that everything will be back to normal.

- Fixes an issue with `yarn audit` when using workspaces

  [6625](yarnpkg/yarn#6639) - [**Jeff Valore**](https://twitter.com/codingwithspike)

- Uses `NODE_OPTIONS` to instruct Node to load the PnP hook, instead of raw CLI arguments

  **Caveat:** This change might cause issues for PnP users having a space inside their cwd (cf [nodejs/node24065](nodejs/node#24065))

  [6479](yarnpkg/yarn#6629) - [**Maël Nison**](https://twitter.com/arcanis)

- Fixes Gulp when used with Plug'n'Play

  [6623](yarnpkg/yarn#6623) - [**Maël Nison**](https://twitter.com/arcanis)

- Fixes an issue with `yarn audit` when the root package was missing a name

  [6611](yarnpkg/yarn#6611) - [**Jack Zhao**](https://github.com/bugzpodder)

- Fixes an issue with `yarn audit` when a package was depending on an empty range

  [6611](yarnpkg/yarn#6611) - [**Jack Zhao**](https://github.com/bugzpodder)

- Fixes an issue with how symlinks are setup into the cache on Windows

  [6621](yarnpkg/yarn#6621) - [**Yoad Snapir**](https://github.com/yoadsn)

- Upgrades `inquirer`, fixing `upgrade-interactive` for users using both Node 10 and Windows

  [6635](yarnpkg/yarn#6635) - [**Philipp Feigl**](https://github.com/pfeigl)

- Exposes the path to the PnP file using `require.resolve('pnpapi')`

  [6643](yarnpkg/yarn#6643) - [**Maël Nison**](https://twitter.com/arcanis)

## 1.12.2

This release doesn't actually exists and was caused by a quirk in our systems.

## 1.12.1

- Ensures the engine check is ran before showing the UI for `upgrade-interactive`

  [6536](yarnpkg/yarn#6536) - [**Orta Therox**](https://github.com/orta)

- Restores Node v4 support by downgrading `cli-table3`

  [6535](yarnpkg/yarn#6535) - [**Mark Stacey**](https://github.com/Gudahtt)

- Prevents infinite loop when parsing corrupted lockfiles with unterminated strings

  [4965](yarnpkg/yarn#4965) - [**Ryan Hendrickson**](https://github.com/rhendric)

- Environment variables now have to **start** with `YARN_` (instead of just contain it) to be considered

  [6518](yarnpkg/yarn#6518) - [**Michael Gmelin**](https://blog.grem.de)

- Fixes the `extensions` option when used by `resolveRequest`

  [6479](yarnpkg/yarn#6479) - [**Maël Nison**](https://twitter.com/arcanis)

- Fixes handling of empty string entries for `bin` in package.json

  [6515](yarnpkg/yarn#6515) - [**Ryan Burrows**](https://github.com/rhburrows)

- Adds support for basic auth for registries with paths, such as artifactory

  [5322](yarnpkg/yarn#5322) - [**Karolis Narkevicius**](https://twitter.com/KidkArolis)

- Adds 2FA (Two Factor Authentication) support to publish & alike

  [6555](yarnpkg/yarn#6555) - [**Krzysztof Zbudniewek**](https://github.com/neonowy)

- Fixes how the `files` property is interpreted to bring it in line with npm

  [6562](yarnpkg/yarn#6562) - [**Bertrand Marron**](https://github.com/tusbar)

- Fixes Yarn invocations on Darwin when the `yarn` binary was symlinked

  [6568](yarnpkg/yarn#6568) - [**Hidde Boomsma**](https://github.com/hboomsma)

- Fixes `require.resolve` when used together with the `paths` option

  [6565](yarnpkg/yarn#6565) - [**Maël Nison**](https://twitter.com/arcanis)

## 1.12.0

- Adds initial support for PnP on Windows

  [6447](yarnpkg/yarn#6447) - [**John-David Dalton**](https://twitter.com/jdalton)

- Adds `yarn audit` (and the `--audit` flag for all installs)

  [6409](yarnpkg/yarn#6409) - [**Jeff Valore**](https://github.com/rally25rs)

- Adds a special logic to PnP for ESLint compatibility (temporary, until [eslint/eslint10125](eslint/eslint#10125) is fixed)

  [6449](yarnpkg/yarn#6449) - [**Maël Nison**](https://twitter.com/arcanis)

- Makes the PnP hook inject a `process.versions.pnp` variable when setup (equals to `VERSIONS.std`)

  [6464](yarnpkg/yarn#6464) - [**Maël Nison**](https://twitter.com/arcanis)

- Disables by default (configurable) the automatic migration of the `integrity` field. **It will be re-enabled in 2.0.**

  [6465](yarnpkg/yarn#6465) - [**Maël Nison**](https://twitter.com/arcanis)

- Fixes the display name of the faulty package when the NPM registry returns corrupted data

  [6455](yarnpkg/yarn#6455) - [**Grey Baker**](https://github.com/greysteil)

- Prevents crashes when running `yarn outdated` and the NPM registry forgets to return the `latest` tag

  [6454](yarnpkg/yarn#6454) - [**mad-mike**](https://github.com/mad-mike)

- Fixes `yarn run` when used together with workspaces and PnP

  [6444](yarnpkg/yarn#6444) - [**Maël Nison**](https://twitter.com/arcanis)

- Fixes an edge case when peer dependencies were resolved multiple levels deep (`webpack-dev-server`)

  [6443](yarnpkg/yarn#6443) - [**Maël Nison**](https://twitter.com/arcanis)
@nzakas

This comment has been minimized.

Copy link
Member

commented Dec 3, 2018

There is an RFC related to this issue: eslint/rfcs#5

@not-an-aardvark

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2019

eslint/rfcs#7, which addresses this issue, has been accepted.

@not-an-aardvark not-an-aardvark added this to Accepted, ready to implement in v6.0.0 Feb 5, 2019

not-an-aardvark added a commit that referenced this issue Feb 13, 2019

Breaking: simplify config/plugin/parser resolution (fixes #10125)
This change updates ESLint to load plugins relative to the user's project root, and other packages relative to where they're specified in a config. This simplifies ESLint's package-loading, resulting in fewer confusing errors about missing packages. It also fixes an existing design bug where ESLint would sometimes fail to load plugins in valid setups.

Implements eslint/rfcs#7.

not-an-aardvark added a commit that referenced this issue Feb 13, 2019

Breaking: simplify config/plugin/parser resolution (fixes #10125)
This change updates ESLint to load plugins relative to the user's project root, and other packages relative to where they're specified in a config. This simplifies ESLint's package-loading, resulting in fewer confusing errors about missing packages. It also fixes an existing design bug where ESLint would sometimes fail to load plugins in valid setups.

Implements eslint/rfcs#7.

@not-an-aardvark not-an-aardvark moved this from Accepted, ready to implement to Implemented, pending review in v6.0.0 Feb 13, 2019

@not-an-aardvark

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2019

I've created a PR to fix this issue in #11388. The PR probably won't be merged for awhile (since it's going to have to wait for our next major release), but if this issue has been causing problems for anyone's integrations/package managers/etc., it might be a good idea to double-check that the implementation in the PR fixes those problems.

not-an-aardvark added a commit that referenced this issue Feb 19, 2019

Breaking: simplify config/plugin/parser resolution (fixes #10125)
This change updates ESLint to load plugins relative to the user's project root, and other packages relative to where they're specified in a config. This simplifies ESLint's package-loading, resulting in fewer confusing errors about missing packages. It also fixes an existing design bug where ESLint would sometimes fail to load plugins in valid setups.

Implements eslint/rfcs#7.

not-an-aardvark added a commit that referenced this issue Feb 22, 2019

Breaking: simplify config/plugin/parser resolution (fixes #10125)
This change updates ESLint to load plugins relative to the user's project root, and other packages relative to where they're specified in a config. This simplifies ESLint's package-loading, resulting in fewer confusing errors about missing packages. It also fixes an existing design bug where ESLint would sometimes fail to load plugins in valid setups.

Implements eslint/rfcs#7.

not-an-aardvark added a commit that referenced this issue Feb 22, 2019

Breaking: simplify config/plugin/parser resolution (fixes #10125)
This change updates ESLint to load plugins relative to the user's project root, and other packages relative to where they're specified in a config. This simplifies ESLint's package-loading, resulting in fewer confusing errors about missing packages. It also fixes an existing design bug where ESLint would sometimes fail to load plugins in valid setups.

Implements eslint/rfcs#7.

v6.0.0 automation moved this from Implemented, pending review to Done Apr 4, 2019

not-an-aardvark added a commit that referenced this issue Apr 4, 2019

Breaking: simplify config/plugin/parser resolution (fixes #10125) (#1…
…1388)

This change updates ESLint to load plugins relative to the user's project root, and other packages relative to where they're specified in a config. This simplifies ESLint's package-loading, resulting in fewer confusing errors about missing packages. It also fixes an existing design bug where ESLint would sometimes fail to load plugins in valid setups.

Implements eslint/rfcs#7.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.