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

Use dependencies instead of peerDependencies. #62

Merged

Conversation

fengmk2
Copy link
Contributor

@fengmk2 fengmk2 commented Oct 28, 2015

npm@3 remove peerDependencies support now.

@fengmk2
Copy link
Contributor Author

fengmk2 commented Oct 28, 2015

npm/npm#6565

npm@3 remove `peerDependencies` support now.
@fengmk2 fengmk2 force-pushed the move-peerDependencies-to-dependencies branch from 06799b1 to d172f77 Compare October 28, 2015 12:55
sokra added a commit that referenced this pull request Nov 23, 2015
…cies

Use dependencies instead of peerDependencies.
@sokra sokra merged commit da039ad into webpack-contrib:master Nov 23, 2015
@sokra
Copy link
Member

sokra commented Nov 23, 2015

Thanks

@jhnns
Copy link
Member

jhnns commented Nov 24, 2015

Sorry, I don't agree with that change. peerDependencies are not deprecated, they are just not installed automatically (which should not have been the case in the first place). In fact, the need for peerDependencies has been confirmed by the npm team.

After maintaining a bunch of compile-to-loaders (less-loader, sass-loader, markdown-loader), I came to the conclusion that it's the best for the maintainer and the developer to use peerDependencies. That is, because the API surface between, say, less and your app is much bigger than between less and the less-loader. It's more practical when the developer is able to choose the exact version of the dependency. Otherwise you'll get a bunch of PRs saying "Plz update less" or "Plz downgrade less because critical bug", etc.

I want to revert that commit.

@zephraph
Copy link

Agreed with @jhnns.

@evocateur
Copy link

I agree with moving peerDependencies into dependencies, because peerDependencies are a world of pain. I wish more packages removed peerDependencies in favor of correct expressions of dependencies.

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.

In this case, since the semver ranges are identical, a consumer of this package can still choose the exact version (as long as it also satisfies ^2.3.1), and the exact version will be the one required by less-loader when called from the consumer's package. Pin less to 2.4.0 in the consumer's top-level dependencies, and npm does the right thing when installing (less@2.4.0 installed in top-level node_modules, and omitted from node_modules/less-loader/node_modules).

If a consumer really cares about controlling the versions of everything in the dependency tree, they should be bludgeoning themselves with using npm shrinkwrap.

@fengmk2 fengmk2 deleted the move-peerDependencies-to-dependencies branch November 25, 2015 02:11
@yiminghe
Copy link

It's not appropriate for less-loader to strongly depend on less, less and its version should be specified by app.

revert +1

sokra added a commit that referenced this pull request Nov 25, 2015
@jhnns
Copy link
Member

jhnns commented Nov 25, 2015

@evocateur you're right, with npm@3 it's probably not that issue anymore.

But interestingly this approach doesn't match with your definition of a dependency:

dependencies are things you require()

If your app says less^2.3.1 in its package.json, there should be a require('less') in your app 😉

@jhnns
Copy link
Member

jhnns commented Nov 25, 2015

@sokra thx

@evocateur @fengmk2 I'm still open for discussion. If you experience actual problems with peerDependencies please let me know. My impression is that it's easier both for the maintainer and the developer.

@fengmk2
Copy link
Contributor Author

fengmk2 commented Nov 26, 2015

'peerDependencies' is work fine on npm2 when the developer forget to add 'less' to their package.json.

But it's broken on npm3 with the same codes.

I'm a private npm registry maintainer for my company.
And a lot of people don't know how peerDependencies work.

@jhnns
Copy link
Member

jhnns commented Nov 26, 2015

I'm a private npm registry maintainer for my company.
And a lot of people don't know how peerDependencies work.

I do understand your pain, but I think it's temporary. With npm@3 you'll get an error when you're trying to install a dependency and you don't have the peerDependency installed. We can also add the peer dependency in our installation note, like npm i less-loader less --save-dev. I did that at the sass-loader as well.

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.

None yet

6 participants