[VBLOCKS-3256] Feat: listen for navigator permissions changes #283
Conversation
kpchoy
force-pushed
the
VBLOCKS-3256/navigator-permissions
branch
from
437cad3 to
aee840f
Compare
kpchoy
force-pushed
the
VBLOCKS-3256/navigator-permissions
branch
from
aee840f to
698148f
Compare
lib/twilio/audiohelper.ts
Outdated
| @@ -275,6 +275,16 @@ class AudioHelper extends EventEmitter { | |||
| if (isEnumerationSupported) { | |||
| this._initializeEnumeration(); | |||
| } | |||
|
|
|||
| navigator.permissions.query({ name: 'microphone' as PermissionName }).then((microphonePermissionStatus) => { | |||
There was a problem hiding this comment.
Do we need to cast 'microphone' as PermissionName here? I don't believe so from my testing unless I'm missing something.
There was a problem hiding this comment.
My VScode is giving my a type error, but it looks like it compiles fine without the typecast 👍
tests/audiohelper.js
Outdated
| navigator.permissions.query = () => Promise.resolve(microphonePermissionStatus); | ||
| audio = new AudioHelper(onActiveOutputsChanged, onActiveInputChanged, {}); |
There was a problem hiding this comment.
It seems like this test might not going through SDK codepaths, unless my understanding is incorrect. If you remove these two lines, I don't think much changes? It seems like you're creating the microphonePermissionStatus object above and then lines 172~173 test that code path?
There was a problem hiding this comment.
I see your point :( . I am having a hard time testing this feature correctly. Any suggestions?
cc: @charliesantos
There was a problem hiding this comment.
Let's get the implementation approved then I'll review the tests 😄
That way, we don't have to keep updating the tests while we review the PR.
kpchoy
requested review from
mhuynh5757 and
charliesantos
and removed request for
mhuynh5757 and
charliesantos
tests/audiohelper.js
Outdated
| navigator.permissions.query = () => Promise.resolve(microphonePermissionStatus); | ||
| audio = new AudioHelper(onActiveOutputsChanged, onActiveInputChanged, {}); |
There was a problem hiding this comment.
Let's get the implementation approved then I'll review the tests 😄
That way, we don't have to keep updating the tests while we review the PR.
There was a problem hiding this comment.
@kpchoy +1 on the implementation. Please check the tests and +1 if you approve.
Contributing to Twilio
Pull Request Details
JIRA link(s):
Description
Reference issue: #228
Burndown
Tested:
Before review
npm testBefore merge