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

optionalDependency on wrtc is unwanted for web projects #14

Closed
guymguym opened this issue Apr 12, 2015 · 3 comments · Fixed by #16
Closed

optionalDependency on wrtc is unwanted for web projects #14

guymguym opened this issue Apr 12, 2015 · 3 comments · Fixed by #16

Comments

@guymguym
Copy link

Hi @feross

Continuing the discussion in node-webrtc/node-webrtc#194 (and the clutter of trying to solve it with --no-optional) - I see another downside of defining wrtc as an optionalDependency of simple-peer.

I have several projects that depends on simple-peer, because it's awesome :) These projects only use it in web pages, but have no interest for the webrtc stack for nodejs, and they get installed on cloud platforms only to serve those pages to users.

Now wrtc is always attempted to install along with simple-peer, and since wrtc is not yet providing binaries for all platforms, this also starts to build from source... and fails on occasion. So I think wrtc is still not yet in a stable status, but even if it was it is definitely not a small nifty library that can be dragged along anyplace with simple-peer (which is a small and nifty library).

Maybe use peerDependency instead so that if anyone wants to use simple-peer with wrtc it will simple depend on both?

Thanks!

@ghost
Copy link

ghost commented Apr 17, 2015

Agreed here (except about peerDependencies, those are just as evil as optionalDependencies). This is an unwanted feature except when I want node support and is highly likely to fail (I still haven't got the build to actually work yet, which fails the entire package install). What about a separate package for node support? optionalDependencies are just not reliable.

An alternative might be to pass wrtc in as a parameter:

var SimplePeer = require('simple-peer')

var peer1 = new SimplePeer({ initiator: true, webrtc: require('wrtc') })
var peer2 = new SimplePeer({ webrtc: require('wrtc') })

Then in the readme just state that if you want node support, you should pass in opts.webrtc yourself. This also makes the backend implementation swappable so that if something better comes along (like a pure js version) it'll be easy to switch.

@ghost ghost mentioned this issue Apr 19, 2015
@feross
Copy link
Owner

feross commented Apr 20, 2015

Version 5.0.0 removes the wrtc dependency. Instead, you must now pass it in like:

var SimplePeer = require('simple-peer')
var wrtc = require('wrtc')

var peer1 = new SimplePeer({ initiator: true, wrtc: wrtc })
var peer2 = new SimplePeer({ wrtc: wrtc })

@guymguym
Copy link
Author

👍

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 a pull request may close this issue.

2 participants