livekit / client-sdk-js

LiveKit browser client SDK (javascript)
https://livekit.io
Apache License 2.0
335 stars 146 forks source link

Don't create data channel of publisher until sending data message #1118

Closed cnderrauber closed 4 months ago

cnderrauber commented 4 months ago

Will not merge this change until sfu server with https://github.com/livekit/livekit/pull/2686 updated as the old server version always requires data channel Enable this optimization if server supports optional publisher datachannel (protocol > 13)

changeset-bot[bot] commented 4 months ago

🦋 Changeset detected

Latest commit: 8ed27d7f03a252a3ee55540425878e18cdd369cb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package | Name | Type | | -------------- | ----- | | livekit-client | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions[bot] commented 4 months ago

size-limit report 📦

Path Size
dist/livekit-client.esm.mjs 79.13 KB (+0.12% 🔺)
dist/livekit-client.umd.js 84.75 KB (+0.11% 🔺)
cnderrauber commented 4 months ago

But, I think this requires a protocol version bump to ensure backwards compatibility.

This is a client change so protocol bump doesn't work for it, the new server can work with old/new clients.

Also, all clients need to do this to take advantage of this optimisation right?

Right

lukasIO commented 4 months ago

as the old server version always requires data channel

what does that mean in terms of backwards compatibility of a client with this change trying to connect to an older server verison?

cnderrauber commented 4 months ago

as the old server version always requires data channel

what does that mean in terms of backwards compatibility of a client with this change trying to connect to an older server verison?

The publisher peerconnection will be considered as failed by sfu.

lukasIO commented 4 months ago

in order to release this with a new protocol version, we'll need to handle updated leave requests as well as those are part of protocol version 13 and have not yet been implemented.

^ this has been done, but might have been unneeded as this PR only checks for server side protocol version.