search

yjs / y-webrtc

WebRTC Connector for Yjs
MIT License
448 stars 109 forks source link

Why keys for webrtcPeers? #17

Closed maxkrieger closed 3 years ago

maxkrieger commented 3 years ago

Following the code in this PR's comment https://github.com/yjs/y-webrtc/pull/6#issuecomment-594532624 doesn't work. provider.on("peers", (e) => sends out an event with no access to the simple-peer objects (webrtcPeers is a list of strings). I have to instead do provider.room.webrtcConns.get(peerId).peer.

It's because of this line:

https://github.com/yjs/y-webrtc/blob/d9f2b28b2ab28edfc11a0b0b48f3bc0a5919f8bf/src/y-webrtc.js#L109

maxkrieger commented 3 years ago

also: I believe due to how defaults are handled, to get video streaming, you need to explicitly set streams: [] for peerOpts if you want to dynamically add the stream later on. Not entirely sure, but worked for me.

dmonad commented 3 years ago

Hi @maxkrieger ,

I added these options only for a hackathon that also wanted to do video streaming over y-webrtc.

My reasoning probably was that I don't want to send the whole webrtcConns object all the time. I only want to send the keys that changed.

It is better in this context to access peers like this: provider.room.webrtcConns.get(peerId).peer, because internally I could change the simple-peer object at any time. So if the event above sends over a Map<PeerID, SimplePeer> then the SimplePeer object might be outdated when you receive this event.

You are indirectly asking if I could change this event to make your use-case easier to handle. That's fair. But I don't have the time to support this use-case - it is unfortunately out-of-scope at the moment..

In any case, your change-request might break existing projects. So it seems like a very bad work/benefit ratio for me.

Btw, it's completely fair if you want to for y-webrtc to handle this use-case better. At some point I might get inspired by your changes and push them upstream. If you made some significant improvements and if you want to help maintain y-webrtc, I'd be happy to push your changes.

maxkrieger commented 3 years ago

Totally reasonable and I hope I didn't sound too standoffish, I've had a great experience with this library and have been recommending it to others.

To me the behavior seemed like a bug (was inconsistent with the recommended usage in that comment) which is why I raised this — since it's relatively undocumented, the solution in my mind is less about breaking API changes than more documentation of the provider's public state, which I would be happy to offer help with if I wasn't so bogged down as well. Regardless, this clarification makes total sense!