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

Remove googConstraints from RTCPeerConnection constructor #1022

Merged
merged 3 commits into from
Feb 5, 2024

Conversation

lukasIO
Copy link
Contributor

@lukasIO lukasIO commented Feb 5, 2024

These constraints have been deprecated in Chromium since M103 and should be simply ignored by Chrome since then.
Users reported errors however (coincidentally with Chrome 103) with the error message Failed to construct 'RTCPeerConnection': Malformed constraints object. Was not able to repro this error using Chrome 103.

Given that this constraint was only ever used for enabling setting local network priority and has been ignored in Chrome for a long time, it seems like it should be ok to remove it entirely and therefore reducing error potential. Removing it will mean manually setting networkPriority will not have any effect on Chrome prior to version 103.

Alternative would be to try/catch RTCPeerConnection instantiation.

Copy link

changeset-bot bot commented Feb 5, 2024

🦋 Changeset detected

Latest commit: 1f6ffbc

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

@lukasIO lukasIO requested a review from davidzhao February 5, 2024 11:19
Copy link
Contributor

github-actions bot commented Feb 5, 2024

size-limit report 📦

Path Size
dist/livekit-client.esm.mjs 80.99 KB (-0.08% 🔽)
dist/livekit-client.umd.js 86.58 KB (-0.09% 🔽)

Copy link
Member

@davidzhao davidzhao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. agreed this is safer from a compatibility standpoint

@lukasIO lukasIO merged commit 88d404b into main Feb 5, 2024
3 checks passed
@lukasIO lukasIO deleted the lukas/remove-goog-constraints branch February 5, 2024 17:38
@github-actions github-actions bot mentioned this pull request Feb 5, 2024
@HermanBilous
Copy link
Contributor

It seems to me that the issue might not have been in passing of deprecated googConstraints, but because of this line:

this.subscriber = new PCTransport(rtcConfig, loggerOptions);

Which should really have been

this.subscriber = new PCTransport(rtcConfig, undefined, loggerOptions);

Otherwise loggerOptions are passed as media constraints. The type for mediaConstraints is wide enough to support that, so ts error is not thrown.

@lukasIO
Copy link
Contributor Author

lukasIO commented Feb 8, 2024

that's a good catch! that might have been the issue. Thanks for pointing that out.
Don't see a need to revert the removal of the googConstraints though. Do you have a usecase that depends on them?

@HermanBilous
Copy link
Contributor

Nah, I'm sure we don't rely on googConstraints, but I still think that going without removal of media constraints is the best option, since we would be able to fix the bug with minimal testing effort. Thanks for fixing it in v1!

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.

3 participants