Skip to content
This repository has been archived by the owner. It is now read-only.

npm link and multiple levels of peerDependencies = psychedelic mess #5080

Closed
mpj opened this issue Apr 15, 2014 · 147 comments
Closed

npm link and multiple levels of peerDependencies = psychedelic mess #5080

mpj opened this issue Apr 15, 2014 · 147 comments

Comments

@mpj
Copy link

@mpj mpj commented Apr 15, 2014

So npm link behaves in (what I think is) an unexpected way when you multiple levels of packages that depend on the same peerDependency.

Look at the dependency chain below. I have a Knockout app, spotify-app, that relies on a spotify-framework module that in turn relies on a spotify-library module. All three depend on knockout and it's very important that there is only one instance of Knockout in the app because reasons. Thus, spotify-app requires knockout as a normal dependency, and the spotify-framework and spotify-library module both list it as a peerDependency. This dependency chain works fine, until you decide to do development on the spotify-framework module and try to use npm link - then the mad hatter comes in and gives you spiked cookies and everything goes pink fluffy unicorns dancing on rainbows.

This is what happens: You happily clone the spotify-framework from git, and then you run npm link in its directory, in preparation of linking it in from spotify-app. The problem here is that npm link will run spm install (I think) under the hood. npm will look at the peerDependencies of spotify-library, and then install an instance of knockout inside the node_modules of spotify-framework, because it thinks that spotify-framework is the top-level module. This will silently cause _duplicate versions_ on knockout, because spotify-app will install another instance of knockout itself. It's obvious what happens when I write it down like this, but it was extremely confusing to figure out what happened here, I tell you.

I'm not sure how this should be solved, but I'm leaning towards that npm link should never install peerDependencies on it's own, because it's most likely not the top-level module if it's being linked.

{
  "name": "spotify-app",
  "version": "0.0.1",
  "private": "true",
  "dependencies": {
    "knockout": "~3.0.0",      
    "spotify-framework": "0.0.1"
  }
}

{
  "name": "spotify-framework",
  "version": "0.0.1",
  "private": "true",
  "peerDependencies": {
        "knockout": "^3.0.0",
  },
  "dependencies": {
    "spotify-library": "0.0.1"
  }
}

{
  "name": "spotify-library",
  "version": "0.0.1",
  "private": "true",
  "peerDependencies": {
        "knockout": "^3.0.0",     
  }
}
@mpj mpj changed the title npm link and multiple levels of peerDependencies npm link and multiple levels of peerDependencies = psychadelic mess Apr 15, 2014
@mpj
Copy link
Author

@mpj mpj commented Apr 15, 2014

Hopefully @domenic can chime in on this.

@domenic
Copy link
Contributor

@domenic domenic commented Apr 15, 2014

Thus, spotify-app requires knockout as a normal dependency, and the spotify-framework and spotify-library module both list it as a peerDependency.

This is not a correct use of peerDependencies. spotify-framework and spotify-library should list knockout in their dependencies list, and then you should use npm dedupe to ensure there is only one copy.

People make this mistake often enough that we should probably add it to the peer dependencies documentation.

@isaacs
Copy link
Member

@isaacs isaacs commented Apr 15, 2014

I'm tempted more and more all the time to say that peerDependencies was just a mistake. It's a departure from The npm Way. It introduces dependency hell, which was so common in every platform except npm, because of how it does things. It is an exposure of that which ought to be encapsulated.

dependencies are things you require(). devDependencies are things you require() for tests and prepublish compilation. If you don't require() it, then it shouldn't be a dependency.

peerDependencies only works for modules like express or grunt that expect to be a singleton monolith. In those cases, one could argue, it's appropriate for them to be singleton monoliths, since they're frameworks that expose some shared plugin surface. However, they're rare enough that they ought to just one-off their checks.

A module like express or grunt should set a global, and throw if it's already set by some other version of the lib. Then plugins can write in their documentation which versions of the core thing they work with. The inherent friction in this approach will guide people towards writing simpler, more modular programs, which encapsulate their dependencies and expose independently useful API surfaces.

@isaacs
Copy link
Member

@isaacs isaacs commented Apr 15, 2014

Another option for plugins would be for the plugin host to document some specific means for determining what version of the host the plugin works with. Then it can do something like this:

var hostVersion = require('./package.json').version;
var semver = require('semver');

exports.addPlugin = function(plugin) {
  var supported = plugin.supported;
  if (!supported)
    throw new TypeError('wtf, doesnt say what it wants: ' + plugin.name);
  if (!semver.satisfies(hostVersion, plugin.supported))
    throw new TypeError('plugin not supported by this host. Wants: ' + plug.supported + ' actual=' + hostVersion);
  this.plugins.push(plugin);
};
@mikeal
Copy link
Contributor

@mikeal mikeal commented Apr 15, 2014

+1 to deprecation. people have been writing more and more about alternative patterns for what people are using peerDeps for and I've been convinced.

@matteofigus
Copy link

@matteofigus matteofigus commented Apr 15, 2014

"However, they're rare enough that they ought to just one-off their checks" -- agree, +1 for deprecation

@maxogden
Copy link

@maxogden maxogden commented Apr 15, 2014

enthusiastic +1 for deprecation. peerDeps have only caused trouble in my experience

@crrobinson14
Copy link

@crrobinson14 crrobinson14 commented Apr 15, 2014

+1 can't live without this. Oh wait. No, I can. Isn't that the litmus test?

@AdrianRossouw
Copy link

@AdrianRossouw AdrianRossouw commented Apr 15, 2014

+1 for removal. They were attractive, but ultimately a mistake.

I think that the entire reason you would want them, is the entire reason they don't work.

The magic sauce of npm is that it allows more specific overrides downstream. Packages shouldn't be able to make assumptions about packages above them in the file hierarchy.

@joaquimserafim
Copy link

@joaquimserafim joaquimserafim commented Apr 15, 2014

👍

@KoryNunn
Copy link

@KoryNunn KoryNunn commented Apr 15, 2014

My impression of what peerDependencies are for is as follows:

Module A absolutely needs module B in order to work, it is a dependency.

BUT

It would be completely incorrect to install module B when installing module A, because module B is to be used as a top-level module, say, a framework or similar.

I've used this pattern a fair bit, and it works perfectly, does not lead to dependency hell, and is extremely easy to understand when reading the code.

It allows 'plugins' to share constructors, meaning the host module can simply do:

if(moduleA.thing instanceof SomeModuleBConstructor){
    // Good to go.
}

I feel that peerDependencies are currently abused, because there is very little documentation around them. The usual case, where peerDependencies are not require()'d, is very confusing, because it is hard to see where the dependency is.

TL;DR, a code example:

// Module A

// peerDependency on Module B

var Ctor = require('moduleB').Ctor;

function InheritedCtor(){}

module.exports = util.inherits(InheritedCtor, Ctor);


// Module B

function Ctor(){}

module.exports.Ctor = Ctor;

Here it is immediately obvious why module B shouldn't be a normal dependency of module A, but that A depends on B being there. This is where peerDependencies are very useful.

@Raynos
Copy link
Contributor

@Raynos Raynos commented Apr 15, 2014

I'm tempted more and more all the time to say that peerDependencies was just a mistake.

I just opened yet another issue of a really hard to debug peer dependencies edge case ( #5084 ).

Peer deps are really hard to implement correctly.

It introduces dependency hell,

That's the exact issue I ran into.

+1 on removing them

@DamonOehlman
Copy link

@DamonOehlman DamonOehlman commented Apr 15, 2014

If it introduces dependency hell, or anything even close to that, then yes I think deprecation is probably the best solution.

@KoryNunn
Copy link

@KoryNunn KoryNunn commented Apr 15, 2014

It probably doesn't help that there is no mention of peer dependencies anywhere in here

It would be unfortunate to remove a useful feature just because people don't know how to use it.

@tonylukasavage
Copy link

@tonylukasavage tonylukasavage commented Apr 16, 2014

+1 for deprecation. grunt for example has grunt-init where these dependencies are prepopulated in a template, so whatever the new pattern is, it should be relatively easy to inject it into newly made plugins at the very least.

@searls
Copy link

@searls searls commented Apr 16, 2014

90% of my node.js adventures are spent maintaining build tools and testing tools. Overall, I'd rate my experience with npm as a B-/C+ when used for maintaining these sorts of projects. Several aspects of the npm way just don't fit particularly well for our work. In particular, inconsistencies with caching, shrinkwrap, dedupe have been frustrating. npm seems designed first for shared libraries, second for deployed applications, and a distant third for tools used to build other projects.

All that's okay, but hearing that folks think peerDependencies was mistake in hindsight seems to reinforce this impression. In fact, peerDependencies are the only logical fit for plugin-driven libraries like grunt, mimosa, and my own lineman. If they were deprecated, we'd be left without an (npm-provided) replacement. That said, we've already had to reinvent a bunch of module resolution stuff, and I suspect we'd just continue to do so.

FWIW, I'm 👎 on deprecating, but I suspect I'll be in the minority.

@substack
Copy link
Contributor

@substack substack commented Apr 16, 2014

I'm strongly in favor of deprecation. The approach that has worked very well in browserify for the transform API is to support extension as much as possible through base javascript primitives and core node abstractions only. For transforms, the signature is:

module.exports = function (file, opts) {
  return transformStream
};

where transformStream is a core node stream. This way transforms don't need to load any code from browserify itself, there is only an implicit contract about what browserify expects and what a transform exports. This means that transforms can be easily reused outside of browserify by tools that know how the interface works, so peerDependencies on transforms would introduce unnecessary coupling on browserify when you're not even using browserify.

The major downside is that it's very hard to change these signatures around so you've got to be careful about making them good enough on the first pass with room to grow without breaking compatibility. I agree with some of the other sentiments here that communicating interface changes should be up to the package with the plugin interface, not npm. When a software library takes on a plugin architecture, it also takes on certain responsibilities for how its ecosystem should evolve in response to changes in the main project.

@thlorenz
Copy link
Contributor

@thlorenz thlorenz commented Apr 16, 2014

I don't think peer deps introduce dependency hell, they just make it explicit.
If I'm a plugin and I depend on being used by a specific version of a parent project, then that's what causes dependency hell.

So we should avoid these situations as much as possible, but if that's not possible, it is better to make it explicit and fail/warn at install time instead of at run time.

Unless we can find another way to express indirect dependencies in a way that fails early, I'm not in favor of deprecating.

Instead we should try to find alternatives that avoid situations that make them necessary and educate developers about them.

However removing peer dependencies just removes the ability to at least explicitly state (admit?) that by using this module you are in risk of experiencing dependency hell.

WRT to @substack's browserify example I actually do want to let people know that my transform is only going to work with browserify >=2. That doesn't prevent it from being used with other tools, but just enforces the version with browserify specifically.

Ergo 👎

@searls
Copy link

@searls searls commented Apr 16, 2014

I don't disagree with @substack's comments either and mercifully lineman's plugins are similarly convention-driven to browserify's.

My worry is really that peerDeps are a useful way to declare incompatible version ranges when a particular plugin is no longer compatible with its host, whether it's because the conventions changed or the provided API. Over the long life of a project it's natural for this to happen, and without npm we'll just have to sort out another way of doing it. (Or doing the same thing manually with @substack's resolve module, which we use liberally as it is). In point of fact, we've had to do this to one of lineman's plugins already when we made a change to one of the core module's defaults.

@carlos8f
Copy link

@carlos8f carlos8f commented Apr 16, 2014

I'm for keeping some form of peerDeps, although I agree with the OP. Perhaps the "magic" could be toned down a little, so npm link doesn't automatically npm install.

peerDependencies only works for modules like express or grunt that expect to be a singleton monolith.

This is sort of missing the point. peerDeps are mainly important to create an "ecosystem" of plugins around such modules. Each plugin needs to define what version(s) of the monolithic module they support, since the same copy is shared between them. Without peerDeps there's no formal way to declare such soft dependencies.

Let's not forget that although express and grunt are the main public modules that use peerDeps, there are privately-hosted modules that have plugin ecosystems too. Removing peerDeps would mean an alternative method for declaring soft dependencies would have to be developed. Or people could still declare peerDeps but you'd need a different set of CLI tools (not npm) for managing them.

@seanami
Copy link

@seanami seanami commented Apr 16, 2014

I just recently used peerDependencies in a pull request on less.js-middleware to work around an issue I was having with trying to extend the custom functions of the copy of less that less-middleware was using. In order to do that, I had to be able to require less directly in my application and then add to less.tree.functions, but it wouldn't have an effect if less-middleware had less as a direct dependency, with its own version in its own nested node_modules. The peerDependencies seemed like the perfect way around that.

However, after reading about npm dedupe (I didn't know it existed!), it seems like this would also be a workable approach. It's a bit more difficult for a library like less-middleware to document how a developer should use it in that case (since they have to run some additional commands after installing and also make less a direct dependency manually instead of just npm installing like normal), but it's not that much more complicated.

I don't know that I have enough broad experience yet to +/-1 for deprecation, but I just wanted to share an instance where I had used it, and how I see potential alternatives working.

@MauriceButler
Copy link

@MauriceButler MauriceButler commented Apr 16, 2014

It is EXTREMELY helpful for plugin / tools development.

I still believe that the feature is just being misunderstood / misused by the many who have been making noise about it.

It doesn't help that documentation such as "this is what it does and if you use it for something else your doing it wrong" does not exist, thus everyone uses it differently and it behaves differently then some believe it should.

Education / documentation / confirmation of how the functionality is designed to work is the answer.

Not removing something because its misunderstood. https://twitter.com/ButlerMaurice/status/456219243223068673

To be absolutely clear I'm 👎 on deprecating

@cpsubrian
Copy link

@cpsubrian cpsubrian commented Apr 16, 2014

This is not a correct use of peerDependencies. spotify-framework and spotify-library should list knockout in their dependencies list, and then you should use npm dedupe to ensure there is only one copy.
A module like express or grunt should set a global, and throw if it's already set by some other version of the lib. Then plugins can write in their documentation which versions of the core thing they work with. The inherent friction in this approach will guide people towards writing simpler, more modular programs, which encapsulate their dependencies and expose independently useful API surfaces.

I definitely need to think about these approaches and how it applies to how I've been using peerDeps.

My use case is the 'huge monolithic framework' one. Our 'plugins' require the central framework module and use it as though it was a global, to listen/emit events, grab configuration (that the framework loaded) and register a few select async 'hooks' (aka 'startup', 'shutdown', etc.). Each plugin is just a light wrapper around one or more '3rd party' modules.

I'm totally prepared to admit there might be a 'better' way to do this.

Simplified example:

// monolithic.js
var EventEmitter = require('events').EventEmitter
   , app = new EventEmitter();

app.conf = // load my conf from a yaml file or whatever.

module.exports = app;
// monolithic-redis.js
var app = require('monolithic')
   , redis = require('redis');

app.redis = redis.createClient(app.conf.redis.port, app.conf.redis.host);
// monolithic-users.js
var app = require('monolithic');

// Make sure our peerDep is loaded.
require('monolithic-redis');

// Expose an api.
app.users = {};
app.users.loadUser = function (id, cb) {
  app.redis.get('users:' + id, cb);
};
// my-cool-website.js
var app = require('monolithic');

// Load all the plugins u want.
require('monolithic-server');
require('monolithic-users');
require('monolithic-videos');

app.start();

Overall, I've liked how 'flat' that approach is. Plugins don't need to be wrapped in functions. PeerDeps yells at you if, in your nest of deps, there are conflicts. Specific load order of modules is handled by node.

Now obviously, you could do something like this (which is sorta how flatiron used to work, I think):

// monolithic-users.js
module.exports = function (app) {
  if (!app.redis) throw new Errors('Oops, you need to manually depend on redis');

  // blah blah blah
};
// my-awesome-app.js
var app = new EventEmitter();
require('monolithic-redis')(app);
require('monolithic-users')(app);

It works, but you won't get errors at the time of npm install and now every plugin has to manually (or through some other tooling) check for and report version mismatches and missing deps.

@Raynos
Copy link
Contributor

@Raynos Raynos commented Apr 16, 2014

If we keep peerDependencies we should invest into fixing outstanding bugs in the current implementation.

the existence of bugs in peerDependencies cause issues for normal users that install some module X that depends on module Y that peer depends on module Z.

It's not reasonable that npm should break subtly for ALL USERS of npm modules because someone somewhere deep in the tree uses peerDependencies.

The existance of peerDependencies currently creates dependency hell for ALL USERS of npm. It has to be "fixed" either way, whether that's an improved implementation or removal of the entire feature.

@techwraith
Copy link

@techwraith techwraith commented Apr 16, 2014

PeerDependencies are great for a subset of use cases, but if you use a module that ran into one of those use cases without knowing about the peerDependency, suddenly you're in dependency hell. I'm +1 on this.

@techwraith
Copy link

@techwraith techwraith commented Apr 16, 2014

To clarify: Removing a feature that helps a small number of people and hurts a larger number of people sounds like a good idea to me.

@KoryNunn
Copy link

@KoryNunn KoryNunn commented Apr 16, 2014

Here is an example of how I am using peerDeps:

https://github.com/KoryNunn/gaffa-set/blob/master/set.js

gaffa-set is a very light module that is to be used by an end-developer to build an application.

  • It makes absolutely no sense to use it without using gaffa.
  • It absolutely needs gaffa in order to work.

peerDeps is great here, because if you tring and install version 1.1.0 of gaffa-set, but you have version 1.0.0 of gaffa, npm will complain. This is a good thing.

if you try installing gaffa-set without having gaffa installed, npm should let you know that it wont work, and that you are probably doing something stupid.

@KoryNunn
Copy link

@KoryNunn KoryNunn commented Apr 16, 2014

"Removing a feature that helps a small number of people and hurts a larger number of people sounds like a good idea to me"

It only hurts people who don't know how to use it. This is because there is no documentation.

The majority of JavaScript developers don't know how .prototype works, and abuse it all the time, should we remove it too?

@gajus
Copy link

@gajus gajus commented Oct 30, 2015

I was still struggling with my React setup using peerDependencies:

Now that peerDependencies are no longer installed for the dependencies, dependencies could not load their peerDependencies. This can be solved using webpack resolve.fallback configuration as I have explained in this SO QA: How to make a linked component peerDepdencies use the equivalent node_modules of the script being linked to?.

@magalhas
Copy link

@magalhas magalhas commented Oct 30, 2015

So actually the issue still persists if the lib being linked have for instance React as a devDependency so that tests can be executed?

@gajus
Copy link

@gajus gajus commented Oct 30, 2015

Correct me if I am wrong, but npm link does not install devDependencies. Even if you npm install; npm link, devDependencies will not be made available to the script being linked.

On Oct 30, 2015, at 20:03, José Magalhães notifications@github.com wrote:

So actually the issue still persists if for instance the lib being linked have for instance React as a devDependency so that tests can be executed?


Reply to this email directly or view it on GitHub.

@rally25rs
Copy link

@rally25rs rally25rs commented Jan 11, 2016

Sorry to rez a closed thread, but wondering if anyone has a recommendation on how to proceed resolving peerDependencies issues upgrading from v2 to v3.

We used peerDependencies (probably incorrectly?) to make sure all our projects used the same version of common libs, so that we didn't have to make sure that 6 projects don't get out of sync in their dependency versions (which they naturally would if devs had to update them all manually).

For example:

Project "common-stuff" has:

  "peerDependencies": {
    "brfs": "^1.4.0",
    "colors": "^1.0.3",
    "grunt": "~0.4.5",
    "grunt-browser-sync": "^1.5.3",
    "grunt-browserify": "^3.2.1",
    "grunt-contrib-clean": "^0.6.0",
    "grunt-contrib-concat": "^0.5.0",
    "grunt-contrib-copy": "^0.7.0",
    "grunt-contrib-handlebars": "~0.10.2",
    "grunt-contrib-jshint": "^0.10.0",
    "grunt-contrib-less": "^0.12.0",
    "grunt-contrib-uglify": "^0.6.0",
    "grunt-contrib-watch": "^0.6.1",
    "grunt-karma": "^0.9.0",
    "grunt-search": "^0.1.6",
    "jshint-stylish": "^1.0.0",
    "karma": "^0.12.25",
    "karma-junit-reporter": "^0.2.2",
    "karma-mocha": "^0.1.9",
    "karma-phantomjs-launcher": "^0.1.4",
    "karma-chrome-launcher": "~0.1.3",
    "load-grunt-config": "^0.16.0",
    "mocha": "^2.0.1",
    "chai": "^1.9.2",
    "hbsfy": "^2.2.1",
    "debowerify": "^1.2.0",
    "handlebars": "^3.0.3",
    "watchify": "3.2.1"
  },

Then project "ProjectOne" just needs:

  "devDependencies": {
    "common-stuff": "git+ssh://git@github.com:MyAccount/common-stuff#master"
  },

And project "ProjectTwo" just needs:

  "devDependencies": {
    "common-stuff": "git+ssh://git@github.com:MyAccount/common-stuff#master"
  },

so if we need to add/remove/update a dependency, we just have to do it in "common-stuff" instead of editing 6 projects individually.

Is there a better why to manage this in npm 3?
At the moment everyone who has upgraded past v2 can no longer build, and I'm hesitant to copy/paste all the dependencies to every individual project because it becomes a maintenance pain for the future...

@jhnns
Copy link

@jhnns jhnns commented Jan 12, 2016

We used peerDependencies (probably incorrectly?)

Yes, I think so :)

Usually, I recommend to actually duplicate the dependency list and add it to "ProjectOne" and "ProjectTwo". It may seem like a maintenance nightmare at first, but consider the other possibility: What if "ProjectOne" needs feature X and therefore needs to update dependency Y which forces you to update the dependency for every other project – which might not be possible in some situations (also known as dependency hell)?

However, there are situations where ProjectOne, ProjectTwo, ProjectThree, etc. are almost identical. In this situation, it is probably a good idea to keep a list of common dependencies. Then it'd move all the dependencies from peerDependencies to dependencies and just re-export every module inside "common-stuff".

All you have to ask yourself if ProjectOne, ProjectTwo, ProjectThree have so much in common that it is worth it to enter dependency hell 😀

@cpsubrian
Copy link

@cpsubrian cpsubrian commented Jan 12, 2016

Also, ask yourself if you even need ProjectOne, ProjectTwo, and ProjectThree to even be different modules. In general, modularity is awesome and very important. Especially with utility modules or very separate pieces of functionality. But in that case the dependencies are likely to differ anyhow. If what you're really building is a framework of some type, you may find splitting the framework up into different pieces is more trouble than it's worth in the long run. This very thing happened with my node framework, cantina. I started off with the noble goal of splitting everything out into its own modules. Core, web, redis, models, validators, vhosts, caching, etc, etc. etc. It heavily uses peer-dependencies to juggling deps between 'plugins' and also the core. Now, updating all the modules is a nightmare and its causing the project to get stale. Since this is a server-side framework there really isn't as much a gain as I thought for splitting it all out. Since I generally use a good portion of the plugins in each project, the whole thing really ought to just be one big beast.

@rally25rs
Copy link

@rally25rs rally25rs commented Jan 12, 2016

Thanks for the replies @jhnns and @cpsubrian

In this case these are all projects that share a common build environment, basically. In our case, all projects share the same Grunt tasks, same unit testing framework, same linter, etc.
This makes all our projects have the same grunt tasks, so as devs move between the projects, all the build processes use the same libraries.

It would be nice if there was a way in package.json to link to/import from another external file, which would be another way to deal with this.

Anyway, looks like I'll be copy/pasting the dependency list around. Thanks again.

@dlongley
Copy link

@dlongley dlongley commented Apr 27, 2016

An update for people like @indexzero and @hueniverse: in node 6.x symlinks to peer dependencies are be preserved (not dereferenced using realpath), so that should make doing things like symlinking to peer dependencies during development more pleasant.

CalebMorris added a commit to CalebMorris/react-moment-proptypes that referenced this issue Dec 10, 2016
From npm3 forward the behevior of peer-dependencies has changed:

The peer-depency in previous versions would download the package if it
wasn't listed in dependencies section of package.json. This led to some
discussion and eventually to the change so that the peers are used
specificly to find conflicts in transitive dependencies and don't
download any packages unless specified in dependencies.

Announcement: http://blog.npmjs.org/post/110924823920/npm-weekly-5

Discussion: npm/npm#5080 (comment)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.