Receive audio only or video only #25
Conversation
leaysgur
changed the title
|
All 36 patterns tested! 🎉 LGTM! We have to discuss which version to add it to. The options I can think of are:
Any other suggestions? |
+1 for option 1. |
| sinon.stub(videoOnlyStream, 'getVideoTracks').returns([{}]); | ||
| }); | ||
|
|
||
| it('should returns correct state with audio and video stream', () => { |
There was a problem hiding this comment.
Thanks for comprehensive test cases! 👍 👍 👍
src/peer/negotiator.js
Outdated
| }; | ||
|
|
||
| const stream = options.stream; | ||
| const hasStream = stream instanceof MediaStream; |
There was a problem hiding this comment.
(This is my preference)
I think using options.stream directly is fine because the name options.stream sounds self-explanatory.
const hasStream = options.stream instanceof MediaStream;There was a problem hiding this comment.
Variable stream is used after this line too for simplicity. But it's ok, I'll fix them.
const stream = options.stream;
const hasStream = stream instanceof MediaStream;
const hasAudioTrack = hasStream ? stream.getAudioTracks().length !== 0 : false;
const hasVideoTrack = hasStream ? stream.getVideoTracks().length !== 0 : false;↓
const hasStream = options.stream instanceof MediaStream;
const hasAudioTrack = hasStream ? options.stream.getAudioTracks().length !== 0 : false;
const hasVideoTrack = hasStream ? options.stream.getVideoTracks().length !== 0 : false;There was a problem hiding this comment.
I see, this depends exactly on personal preference; I just prefer cutting assigning variable sometimes.
src/peer/negotiator.js
Outdated
| state.video = true; | ||
| } | ||
|
|
||
| // If stream has track, ignore options |
There was a problem hiding this comment.
Could add the comment that expresses having stream track leads to sendrecv like below?
// If stream has track, ignore options, which results in setting sendrecv internally.
The reason of my comment is just for code readability and wants to stimulate my memory when I'll read the code in the future.
|
LGTM 👍 |
+1 to this option too 😸 |
|
LGTM |
refs #23