Why is plugin-import is peerDep of node-resolver?

[jasonkarns]

I am tracking down some very strange shrinkwrap issues caused by eslint/eslint-plugin-import and I came across this:

https://github.com/benmosher/eslint-plugin-import/blob/71afde73b8d18a487671d1d4ae0534c0ca52e938/resolvers/node/package.json#L31

Why is eslint-plugin-import (hereafter "import") listed as a peerDep of eslint-import-resolver-node (hereafter "resolver")? Is my understanding correct that in no scenario does resolver expect to be a sibling of import?

Resolver is already a direct dependency of import. And resolver does not require or rely on any of the functionality of import to function.

[benmosher]

https://nodejs.org/en/blog/npm/peer-dependencies/#the-problem-plugins

TL;DR: enforcing that the resolver is used only with versions of import that should work well together.

That post is fairly dated, though. Is there a better (or maybe just newer?) way to ensure compatibility?

[jasonkarns]

resolver isn't a plugin for (nor a peer of) import, it's a direct dep. As such, the way to ensure compatibility is already being done: import specifies precisely which version(s) of resolver it requires: https://github.com/benmosher/eslint-plugin-import/blob/71afde73b8d18a487671d1d4ae0534c0ca52e938/package.json#L75

peerDep is only to help ensure compatibility of modules that will be peers (hence the name) which is only useful for modules that are plugins for (installed as siblings of) the respective peerDep. (like eslint-plugin-import is installed as a peer of eslint)

[jasonkarns]

I should also note that, because npm2 actually installs peerDependencies as siblings (rather than simply emitting warnings as npm3 does), it is common with resolve having import as a peerDep, npm will actually install import as a sibling to resolve. And since resolve is a direct dep of import, that means the final node_modules on disk have import as a dependency of itself (within its own node_modules).

Such a scenario:

eslint-plugin-import@1.8.0 /private/tmp/eslint-bug/node_modules/eslint-plugin-import
├─ ...
├─┬ eslint-import-resolver-node@0.2.1
│ └── resolve@1.1.7
├─┬ eslint-plugin-import@1.10.2
│ ├── contains-path@0.1.0
│ ├── UNMET DEPENDENCY eslint@2.x - 3.x
│ ├─┬ lodash.findindex@4.4.0
│ │ ├── lodash._basefindindex@3.6.0
│ │ └─┬ lodash._baseiteratee@4.7.0
│ │   └─┬ lodash._stringtopath@4.8.0
│ │     └── lodash._basetostring@4.12.0
│ └─┬ pkg-dir@1.0.0
│   └─┬ find-up@1.1.2
│     ├── path-exists@2.1.0
│     └─┬ pinkie-promise@2.0.1
│       └── pinkie@2.0.4
├─...

[jasonkarns]

I've been able to boil this down to a minimal reproducible issue, that is actually causing production problems for us: (only manifests on npm2)

npm init -f; npm install --save eslint-plugin-import@1.8.0 eslint@^2.9.0; npm shrinkwrap; rm -r node_modules; npm install; npm shrinkwrap; rm -r node_modules; npm install; npm shrinkwrap; rm -r node_modules; npm install;

The above demonstrates a scratch package with direct deps installed: eslint ^2.9.0 and eslint-plugin-import@1.8.0

Given such a package, running the following commands: npm shrinkwrap; rm -r node_modules; npm install will fail on the third install due to versioning conflicts of eslint:

npm ERR! install trying to install 3.0.1 to /private/tmp/take2/node_modules/eslint
npm ERR! install but already installed versions [ '2.13.1' ]
... [100s of lines snipped]

The cyclical dependency issue of this plugin was not causing a problem until this commit a little over a week ago: f1c420e

[benmosher]

Ah, wild. There were a handful of issues that I never understood around this.

The idea originally was that all the resolvers would be plugins, but then I bundled the Node one because I figured that it covered most use cases. My hope was that it could still be installed as a peer to choose a different version than the bundled one.

I don't think that actually makes sense, as you've clearly stated. Thanks!

@jfmengels: feels like a crossroads between making the node resolver fully optional vs removing the peer dependency. What do you think?

I'm tempted to stop bundling it and instead to have the recommended config reference it, but then it's one more step for the average user. So I'm not sure which direction is more practical.

[jasonkarns]

Ah, wild.

Indeed. SOOO many things had to line up just perfectly for this scenario. (npm2, shrinkwrap, eslint required as a production dep, plugin-import accepting eslint 2 and 3, dependent package requiring eslint 2 but not 3) insanity

feels like a crossroads between making the node resolver fully optional vs removing the peer dependency. What do you think?

Personally I think the optimal ending scenario would be having the node-resolver still included as a direct dependency as it is now, while allowing end users to provide another resolver of their choosing via configuration. (I would model this how most test frameworks/linters etc have bundled or direct-dep reporters, but still allow custom reporters to be referenced via config.)

However, and I think this is the critical part, regardless of whether the node-resolver is left as a direct-dep or fully extracted/made optional; the peerDep listing is having no effect other than to cause plugin-import to be installed into its own node_modules. The removal of the peerDep line from the node resolver can be done without any other changes and the final decision on bundling/configuring can be made separately.

If you decide to go with having end-users choose the resolvers, then it will need extracted and at that point the peerDep line would be appropriate. But until then, it is adding no value while blocking shrinkwrap installs.

[benmosher]

Fair enough. I will get a patch out this morning.

[jfmengels]

Personally I think the optimal ending scenario would be having the node-resolver still included as a direct dependency as it is now, while allowing end users to provide another resolver of their choosing via configuration

This sounds good to me

[benmosher]

Yeah, I think that's right.

[benmosher]

Just published v0.2.2 of Node resolver to npm, with #438 merged. Should be compatible back to roughly v1.4 of the import plugin.

Thanks for this, @jasonkarns! 😁

[jasonkarns]

Thank you @benmosher!