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

set co as peer dependency #11

Closed

Conversation

antoniobusrod
Copy link

No description provided.

@mciparelli
Copy link
Owner

I appreciate the effort but I don't think setting co as a peer dependency would work here. Check out the example in the Readme. The way this library works, you can totally use co-express in your app without having co installed in your system. This wouldn't make much of a sense unless we update implementation.

@mciparelli mciparelli closed this Mar 12, 2016
@antoniobusrod
Copy link
Author

Hello @mciparelli, since co is installed from last version from 4.x, there is no problem. But in the case they upgrade it and you want to use co-express with v5.x, it won't be possible. Besides, there are projects where you want to handle generator based control flow from different places, for example, from a JS CLI script. In that case, you will have same dependency installed twice.

@mciparelli
Copy link
Owner

I won't put co as a peerDep because it doesn't correspond. I mean, if I do, every user currently using co-express without co in their app won't be able to run their app because npm will fail saying co is not installed or similar.

Now if you want to introduce a breaking change in API that switches usage from wrap(generator) to something that looks more like a plugin, e.g. wrap(co(generator)) I'm ok with using a peerDep there. And it's actually probably better that way (although it involves a little bit more code) because this library is really intended as a plugin for co and express. So feel free to send a PR if you got what I'm saying, otherwise I'll try to explain further.

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