This repository has been archived by the owner on May 10, 2024. It is now read-only.
[RTC-226] Remove peers #29
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Rados13
approved these changes
May 25, 2023
src/membraneWebRTC.ts
Outdated
|
|
||
| /** | ||
| * Called when peer was accepted. Triggered by {@link MembraneWebRTC.join} | ||
| * Called when endpoint is ready. Triggered by {@link MembraneWebRTC.connect} |
There was a problem hiding this comment.
Suggested change
| * Called when endpoint is ready. Triggered by {@link MembraneWebRTC.connect} | |
| * Emitted when endpoint is ready. Triggered by {@link MembraneWebRTC.connect} |
src/membraneWebRTC.ts
Outdated
Comment on lines
255
to
257
| * Emitted each time endpoint is removed, emitted both for the local endpoint and other endpoints. | ||
| */ | ||
| onPeerLeft?: (peer: Peer) => void; | ||
| endpointRemoved: (endpoint: Endpoint) => void; |
There was a problem hiding this comment.
Shouldn't we have a proper event for a situation when the local endpoint is removed? Because with the current implementation, a client would have to store in his code what is its endpointId, which IMO is a little bit inconvenient.
src/membraneWebRTC.ts
Outdated
| }); | ||
| this.sendMediaEvent(mediaEvent); | ||
| }; | ||
|
|
||
| /** | ||
| * Updates the metadata for a specific track. | ||
| * @param trackId - trackId (generated in addTrack) of audio or video track. | ||
| * @param trackMetadata - Data about this track that other peers will receive upon joining. | ||
| * @param trackMetadata - Data about this track that other endppoint will receive upon being added. |
There was a problem hiding this comment.
Suggested change
| * @param trackMetadata - Data about this track that other endppoint will receive upon being added. | |
| * @param trackMetadata - Data about this track that other endpoint will receive upon being added. |
Rados13
reviewed
May 25, 2023
src/membraneWebRTC.ts
Outdated
|
|
||
| /** | ||
| * Called when peer was accepted. Triggered by {@link MembraneWebRTC.join} | ||
| * Emitted when endpoint of this peer is ready. Triggered by {@link MembraneWebRTC.connect} |
There was a problem hiding this comment.
I think there should be no peer phrases in this library.
Rados13
approved these changes
May 25, 2023
mickel8
approved these changes
May 25, 2023
| } | ||
|
|
||
| interface TrackContextCallbacks { | ||
| export interface TrackContextEvents { |
There was a problem hiding this comment.
I am not TS magician but using interface as "Enum" looks a little bit weird, or not really?
There was a problem hiding this comment.
Tough to say, although that's how it was done in jellyfish-ts-client.
src/membraneWebRTC.ts
Outdated
| } | ||
|
|
||
| /** | ||
| * Main class that is responsible for connecting to the RTC Engine, sending and receiving media. | ||
| */ | ||
| export class MembraneWebRTC extends (EventEmitter as new () => TypedEmitter<Required<Callbacks>>) { | ||
| export class MembraneWebRTC extends (EventEmitter as new () => TypedEmitter< |
There was a problem hiding this comment.
I think it's a good time to change this name to something new. WebRTCEndpoint comes to my mind 🤔
kamil-stasiak
approved these changes
May 26, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Removing peers to match changes in
membrane_rtc_engine.Introduces many breaking changes.
Additionally:
MembraneWebRTCcompletely in favour of emitted events.Matching PR in
membrane_rtc_engine.