-
Notifications
You must be signed in to change notification settings - Fork 727
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 usedevices from uselocaltracks #508
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good! just one small change to a test description ( i think ), and a couple of typescript questions i had 👍
src/utils/index.test.ts
Outdated
expect(result.hasAudioInputDevices).toBe(false); | ||
}); | ||
|
||
it('should return hasAudioInputDevices: false when there are no audio input devices', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be 'should return hasAudioInputDevices: true when an audio input device is present'
or something like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. This description is a duplicate of the other one. I updated it so it is correct: 'should return hasVideoInputDevices: false when there are no video input devices'
Nice catch!
src/hooks/useDevices/useDevices.tsx
Outdated
import { getDeviceInfo } from '../../utils'; | ||
|
||
// This returns the type of the value that is returned by a promise resolution | ||
type ThenArg<T> = T extends PromiseLike<infer U> ? U : T; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this saying, if the type extends PromiseLike, then use the type from the value from the resolved Promise, otherwise use the original type T? I'm not sure i've used the infer
keyword before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's correct! It's odd, but I think that this ternary notation is really the only way to extract the value of the promise with infer
. I couldn't find anything better on stack overflow.
One thought I had, is that we're never going to need to return T
if the value isn't PromiseLike. So we can change this just to show that we don't intend on using this type in that way:
type ThenArg<T> = T extends PromiseLike<infer U> ? U : never;
|
||
// First, get tracks | ||
await act(async () => { | ||
result.current.getAudioAndVideoTracks(); | ||
await waitForNextUpdate(); | ||
await waitForValueToChange(() => result.current.localTracks.length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'll need to remember waitForValueToChange()
, i haven't seen that before!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's nice! I haven't used it much before. There's also this
waitFor(() => result.current.localTracks.length > 0);
It will wait until the condition is true
. It only exists in a newer major version of the testing library. I tried upgrading, but it broke things. We can upgrade it at another time.
Contributing to Twilio
Pull Request Details
JIRA link(s):
Description
This pull request removes the usage of the
useDevices
hook from theuseLocalTracks
hook.The values in the
useDevices
hook are not updated after a user grants permission to their microphone and camera. Because of this, the values returned byuseDevices
can be stale by the time they are used in the callbacks insideuseLocalTracks
.This issue is solved by calling
enumerateDevices
at the beginning of the execution of theuseLocalTracks
callbacks. With this, the device information is never stale.Resolves: #425
Burndown
Before review
npm test
Before merge