karma-runner / karma

Spectacular Test Runner for JavaScript
http://karma-runner.github.io
MIT License
11.96k stars 1.71k forks source link

Dependencies versus PeerDependencies in plugins dependencies #1166

Closed aymericbeaumet closed 10 years ago

aymericbeaumet commented 10 years ago

This topic will be more appropriate to continue the discussion initially started here: https://github.com/karma-runner/karma-traceur-preprocessor/pull/3#issuecomment-52361104


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

peerDependencies

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 commented 10 years ago

@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 commented 10 years ago

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 commented 10 years ago

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 https://github.com/google/traceur-compiler/issues/915 gets figured out of course.

pkozlowski-opensource commented 10 years ago

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 commented 10 years ago

Interestingly, https://github.com/sindresorhus/gulp-traceur pins the version of traceur: https://github.com/sindresorhus/gulp-traceur/blob/master/package.json, Grunt plugin does the same: https://github.com/aaronfrost/grunt-traceur/blob/master/package.json

even if the API used is pretty minimal, ex.: https://github.com/sindresorhus/gulp-traceur/blob/master/index.js#L34

caitp commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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

caitp commented 10 years ago

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

aymericbeaumet commented 10 years ago

@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:

Which benefits do you see to omit the line?

domenic commented 10 years ago

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

aymericbeaumet commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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

@domenic basically 2 things:

Anyway, I can see that you've moved to using peer deps (https://github.com/karma-runner/karma-traceur-preprocessor/commit/9f2869161dc561332ce0eae3f1a8ede57aad2ddc) so I guess we can close this issue :-)

aymericbeaumet commented 10 years ago

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

pkozlowski-opensource commented 10 years ago

Yeh, personally I would be interested in having gulp moving to peer deps: https://github.com/sindresorhus/gulp-traceur/issues/31

aaronfrost commented 10 years ago

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 ;) )