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

feat: added ability to specify id for pc #21

Merged
merged 4 commits into from
May 16, 2024

Conversation

IlgamGabdullin
Copy link
Contributor

Discussed issue here

Added new optional id?: string param in handleNewPeerConnection method and emited the id back in callback calculateNetworkScores, so now developers can use it that way

webRTCIssueDetector.handleNewPeerConnection(pc, 'some-id-1')
webRTCIssueDetector.handleNewPeerConnection(pc, 'some-id-2')
webRTCIssueDetector.handleNewPeerConnection(pc, 'some-id-3')

and in callback it wil be

onNetworkScoresUpdated: (scores) => scores.id; // some-id-X

@evgmel @desher @panov-va please take a look

@desher
Copy link
Contributor

desher commented May 14, 2024

@IlgamGabdullin thank you for the PR.
I just want to suggest you to simplify the solution. Actually PeerConnection identifiers are not a part of this library so it is better to provide wrapped RTCPeerConnection object with inner id property. In total all you need to do here is add pc property to NetworkScores(onNetworkScoresUpdated callback payload).

@evgmel
Copy link
Collaborator

evgmel commented May 14, 2024

@desher Good idea 👍 @IlgamGabdullin , don't you mind making this changes in your PR?

@IlgamGabdullin
Copy link
Contributor Author

@desher thank you for the idea!
I've pushed commits with your solution, seems to be working. Please take a look if I understood you correctly

@desher
Copy link
Contributor

desher commented May 16, 2024

Awesome! Thank you @IlgamGabdullin
There's only one thing left for improvement. Please rename pc to connectionId to avoid possible confusion.

@desher desher merged commit 95d17d4 into VLprojects:master May 16, 2024
2 checks passed
@vlprojects-bot
Copy link
Contributor

🎉 This PR is included in version 1.10.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants