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

Added support for peerDependencies in the package.json file #219

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

heath-guidewire
Copy link

  • haven't fully tested all commands yet

- haven't fully tested all commands yet
@amilajack amilajack requested a review from LinusU February 23, 2017 22:27
@LinusU
Copy link
Collaborator

LinusU commented Feb 24, 2017

🤔 but peerDependencies shouldn't be installed, right? It's for indicating what version packages depending on your package must use?

e.g. if you have a middleware that only work with version 2 of express.js, but your code itself doesn't require('express'), than you should specify a peerDependency on express 2.x

@heath-guidewire
Copy link
Author

heath-guidewire commented Feb 24, 2017

True... BUT npm-check doesn't deal directly with installing dependencies... it's about checking the peer dependencies, making sure they are all used, making sure they are also up-to-date (and optionally helping you update them if they aren't), letting you know whether they are actually installed. This commit allows npm-check to help people do that for all 3 of the relevant dependency blocks provided by NPM, not just the two most common ones.

At my work we are building some internal 'open source' code for the rest of the company, and we are using 'peer dependencies' in our NPM modules package.json files for the modules that our code relies on, so that users can make sure they have the correct ones. These changes will allow us AND other npm module authors to keep their peerDependencies up-to-date.

@LinusU
Copy link
Collaborator

LinusU commented Feb 25, 2017

making sure they are all used

As I have understood peer dependencies you shouldn't require() anything that is only in those dependencies. It's used for when you are being passed or are passed to another library. If anything, I think that npm-check should check that you aren't requireing your peer dependencies...

@heath-guidewire
Copy link
Author

heath-guidewire commented Feb 28, 2017

There is some debate in my company about peerDependencies in relation to how they are meant to be used. Right now the prevailing opinion is that they represent 3rd party packages required by our packages that the user needs to make sure are installed. If you look at other library developers, like ReactStrap (https://github.com/reactstrap/reactstrap), they are also using peerDependencies in a similar way. While I'm enjoying the debate, I think that my PR makes sense for how some devs might use it... If you want to have peerDependencies be a list of things that should not be require()d, then I welcome you to enhance my changes with a flag that will treat them in that manner.

@LinusU
Copy link
Collaborator

LinusU commented Mar 6, 2017

While I'm enjoying the debate, I think that my PR makes sense for how some devs might use it...

Well the problem is that all those projects will start to fail if we merge this PR... also, I'm yet to be convinced that that is the right way to use them. I think that we should first settle how they should be used, and then add that behaviour behind a flag, so that we don't break anyone. It could possibly be enabled by default in the next major version also :)

@heath-guidewire
Copy link
Author

So, assuming I understand you correctly, backwards compatibility is such that the flag should be to turn it on, rather than off like I suggested?

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

Successfully merging this pull request may close these issues.

2 participants