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

Dependencies versus PeerDependencies in plugins dependencies #1166

Closed
aymericbeaumet opened this issue Aug 20, 2014 · 19 comments
Closed

Dependencies versus PeerDependencies in plugins dependencies #1166

aymericbeaumet opened this issue Aug 20, 2014 · 19 comments
Labels

Comments

@aymericbeaumet
Copy link
Member

This topic will be more appropriate to continue the discussion initially started here: karma-runner/karma-traceur-preprocessor#3 (comment)


This is a tricky issue and I am not sure there is a perfect solution. We just need to choose one and stick with it. Here's how I see the problem:

dependencies

  • The scope brought by dependencies allows a one to use several node packages using different versions of a common dependency.
  • It also guarantees that the packaged version effectively works along with the plugin (if tests are properly implemented).
  • The version should be updated manually.

peerDependencies

  • Use the latest version of the dependency.
  • It could be in conflict with another package requiring a different version of the same peer dependency.
  • Compatibility could silently break if semver is not rigorously respected

IMHO, the advantage brought by peerDependencies to update to the latest version doesn't offset the benefits and the security brought by dependencies.

@pkozlowski-opensource
Copy link
Member

@aymericbeaumet while reading through your comments I'm getting the impression that one of us is miss-interpreting behavior of peerDependencies. IMO you can specify the same version range for peer dependencies as for dependencies. In other words a plugin still got control over version of its dependencies and doesn't necesirrly need to work with the latest version. In the light of this here are my comments to your assesement:

The scope brought by dependencies allows a one to use several node packages using different versions of a common dependency.

True, but IMO we want the oposite, this is what the whole issue is all about

It also guarantees that the packaged version effectively works along with the plugin (if tests are properly implemented).

I don't think peer dependencies change much here, a plugin can still express semver range with witch it works correctly.

The version should be updated manually.

Not sure I got you here... If a project doesn't specify any version of a given dependency npm will install the latest version specified in the semver range

Use the latest version of the dependency.

I don't think it is true....

It could be in conflict with another package requiring a different version of the same peer dependency.

Yes, but at least we would get clear info from npm during the instalation time instead of tracking weired runtime errors in a browser.

Compatibility could silently break if semver is not rigorously respected

Not sure I got your point....

It might be very well that I'm completly missing the point of peer deps but IMO those are the soultion for things like traceur where we would use the same version to transpile code, run tests and use during runtime.

@aymericbeaumet
Copy link
Member Author

My bad, peerDependencies allows the same semver range as dependencies, I misunderstood that point. I striked through irrelevant points in my first post.

It might be very well that I'm completly missing the point of peer deps but IMO those are the soultion for things like traceur where we would use the same version to transpile code, run tests and use during runtime.

From this perspective the use of peerDependencies would be clearly adapted. But IMO this is not applicable to a majority of Karma plugins.

I just want to avoid it when not absolutely necessary because it could really be a potential source of frustrations for users, for example: oh snap, the latest version of plugin A doesn't work along with plugin B because the common peer dependency D had a breaking change not yet implemented by B. So I have to use a 3 months old version of A to fulfil the peer dependency because of B.

@domenic
Copy link

domenic commented Aug 21, 2014

So, the case for peer dependencies is basically that it would allow "bring your own Traceur" (for example). That is, if karma-traceur-preprocessor depends on Traceur, and so does my project which uses Traceur for compilation, then we have two different versions of Traceur in action: one for running the tests with Karma, and one for running my actual app. Whereas if karma-traceur-preprocessor peer-depends on Traceur, and my app depends on Traceur and on karma-traceur-preprocessor, then karma-traceur-preprocessor will pick up my app's copy, instead of using its own.

As for the "oh snap" scenario, it's a little hard to tell what the actual problem would be given the abstract A, B, and D, but from what I can see dependencies would not solve the problem, and would just result in incompatible versions of D wreaking havoc through your project and/or your tests.

That said, there's an additional wrinkle when talking about Traceur, or indeed any pre-1.0 tool: without a semver guarantee of compatibility, the "right" thing to do is just to lock your peer-dependency on Traceur to the known-working versions. But then karma-traceur-preprocessor becomes incompatible with your app when your app wants to upgrade Traceur, and we have to go bug the publishers of karma-traceur-preprocessor to upgrade their known-safe-versions list in package.json and republish. At the other end of the spectrum, we could just have it peer-depend on e.g. "traceur": "*", but then we lose pretty much all of the benefits of peer-dependencies since we're not doing any checking that they are using a compatible Traceur version.

In that particular case, I might suggest something more like: no dependency or peer-dependency, but try to load the file at runtime and check that the API works as expected; if it does not fail loudly. That is only a bridge until google/traceur-compiler#915 gets figured out of course.

@pkozlowski-opensource
Copy link
Member

From this perspective the use of peerDependencies would be clearly adapted. But IMO this is not applicable to a majority of Karma plugins.

@aymericbeaumet I was never suggesting that we should move all the Karma deps to peer deps - peer deps make sense only when there are multiple consumers of the same library in the same project and all those consumers want / must agree on a specific version. IMO this is exactly the case for traceur (as the "interested consumers" are: Karma, build tool, runtime) and this is why I've started the discussion in the Traceur repo.

My proposal: let's focus the discussion on the traceur (and move the discussion back to https://github.com/karma-runner/karma-traceur-preprocessor?).

+1 for @domenic suggestion to commit to a minimal public API that is meaningful to the external tools (karma, gulp / grunt etc.) so we can take advantage of the semver "safety".

@pkozlowski-opensource
Copy link
Member

@caitp
Copy link

caitp commented Aug 21, 2014

Vojta has a version of gulp-traceur which does use peerDeps, https://github.com/vojtajina/gulp-traceur/blob/traceur-as-peer

But interestingly we aren't using it in angular/pipe (well, not strictly true, some of the angular 2 packages are using it)

@domenic
Copy link

domenic commented Aug 21, 2014

Yeah, this is somewhat of a Traceur-specific problem, since it is pre-1.0 but many tools depend on the fairly-stable traceur.compile API. Thus it is tempting to not use dependencies or peer dependencies at all (or use a peer dependency with version *, which is basically equivalent).

@pkozlowski-opensource
Copy link
Member

Agreed. karma-traceur processing is also using only traceur.compile:
https://github.com/karma-runner/karma-traceur-preprocessor/blob/master/index.js#L22

For me peerDependencies: {traceur: '*'} sounds like a good option.

@domenic
Copy link

domenic commented Aug 21, 2014

What benefit do you see of peerDependencies: {traceur: '*'} over omitting that line entirely?

@caitp
Copy link

caitp commented Aug 21, 2014

explicitly saying "we need this", instead of just leaving it up to you to figure it out from the stack trace of whatever exception gets thrown, probably

domenic added a commit to karma-runner/karma-traceur-preprocessor that referenced this issue Aug 21, 2014
@aymericbeaumet
Copy link
Member Author

@pkozlowski-opensource

I was never suggesting that we should move all the Karma deps to peer deps

I know :)


@domenic

What benefit do you see of peerDependencies: {traceur: '*'} over omitting that line entirely?

By entirely removing the line from the package.json you would have to indicate somewhere that the dependency must be separately provided (npm install traceur karma-traceur-preprocessor). It's just counter-intuitive IMO.

peerDependencies: {traceur: '*'} has the advantage to be very flexible:

  • there is no constraints whatsoever on the Traceur's version
  • a package installation by itself (without manually installing Traceur) works

Which benefits do you see to omit the line?

@domenic
Copy link

domenic commented Aug 21, 2014

People should already be using traceur, and shouldn't count on karma-traceur-preprocessor to install it for them.

@aymericbeaumet
Copy link
Member Author

I got your point. But you are assuming that as they need Traceur, people will have it installed as a first-level dependency.

They could use it throught grunt-traceur or gulp-traceur. In such case karma-traceur-preprocessor will not be able to access it. Unless I'm missing something?

@domenic
Copy link

domenic commented Aug 21, 2014

In that case they're screwed: they'll have two versions of Traceur in their project (one top-level, installed by karma-traceur-preprocessor as a peer, and the other underneath grunt-traceur). This is why I think no projects should be taking on traceur as a dependency.

@aymericbeaumet
Copy link
Member Author

no projects should be taking on traceur as a dependency

I follow you on this. But I think they should still peer depend *, semantically speaking.

@pkozlowski-opensource
Copy link
Member

What benefit do you see of peerDependencies: {traceur: '*'} over omitting that line entirely?

@domenic basically 2 things:

  • explicitly stating what you depend on, pretty much the same point as @caitp is making
  • throw-away projects where I do experiment with things with unit tests - in this case it saves me one install

Anyway, I can see that you've moved to using peer deps (karma-runner/karma-traceur-preprocessor@9f28691) so I guess we can close this issue :-)

@aymericbeaumet
Copy link
Member Author

He now has to continue his crusade among the other *-traceur plugins ;)

@pkozlowski-opensource
Copy link
Member

Yeh, personally I would be interested in having gulp moving to peer deps: sindresorhus/gulp-traceur#31

@aaronfrost
Copy link

Might consider having a traceur-tool manifesto that can be hosted on the traceur repo, so that future tools that integrate with traceur can follow this same model, based on this same thought train. That way we might not have to re-conquer having a bunch of conversations again with a new set of tool authors in the future. (not referring to the authors as tools, as I am myself one of those authors. rather referring to their projects as tools ;) )

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

No branches or pull requests

5 participants