VBLOCKS-1111 | Fix sound definitions (twilio#153)

* VBLOCKS-1534 | Stop using deprecated RTCIceCandidateStats * VBLOCKS-1111 | Fix sound definitions * Tests and changelog * Update docs
podium · Mar 31, 2023 · 9de4ceb · 9de4ceb
1 parent e0d4b3c
commit 9de4ceb
Show file tree

Hide file tree

Showing 7 changed files with 134 additions and 53 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -13,6 +13,8 @@ Bug Fixes
 
 - Fixed an [issue](https://github.com/twilio/twilio-voice.js/issues/100) where a `TypeError` is thrown after rejecting a call then invoking `updateToken`.
 
+- Fixed an [issue](https://github.com/twilio/twilio-voice.js/issues/14) where `device.audio.disconnect`, `device.audio.incoming` and `device.audio.outgoing` do not have the correct type definitions.
+
 2.3.2 (February 27, 2023)
 ===================
 

diff --git a/lib/twilio/audiohelper.ts b/lib/twilio/audiohelper.ts
@@ -90,6 +90,15 @@ class AudioHelper extends EventEmitter {
    */
   private _audioContext?: AudioContext;
 
+  /**
+   * Whether each sound is enabled.
+   */
+  private _enabledSounds: Record<Device.ToggleableSound, boolean> = {
+    [Device.SoundName.Disconnect]: true,
+    [Device.SoundName.Incoming]: true,
+    [Device.SoundName.Outgoing]: true,
+  };
+
   /**
    * The `getUserMedia()` function to use.
    */
@@ -172,10 +181,6 @@ class AudioHelper extends EventEmitter {
     this.isOutputSelectionSupported = isEnumerationSupported && isSetSinkSupported;
     this.isVolumeSupported = isAudioContextSupported;
 
-    if (options.enabledSounds) {
-      this._addEnabledSounds(options.enabledSounds);
-    }
-
     if (this.isVolumeSupported) {
       this._audioContext = options.audioContext || options.AudioContext && new options.AudioContext();
       if (this._audioContext) {
@@ -287,6 +292,36 @@ class AudioHelper extends EventEmitter {
     }
   }
 
+  /**
+   * Enable or disable the disconnect sound.
+   * @param doEnable Passing `true` will enable the sound and `false` will disable the sound.
+   * Not passing this parameter will not alter the enable-status of the sound.
+   * @returns The enable-status of the sound.
+   */
+  disconnect(doEnable?: boolean): boolean {
+    return this._maybeEnableSound(Device.SoundName.Disconnect, doEnable);
+  }
+
+  /**
+   * Enable or disable the incoming sound.
+   * @param doEnable Passing `true` will enable the sound and `false` will disable the sound.
+   * Not passing this parameter will not alter the enable-status of the sound.
+   * @returns The enable-status of the sound.
+   */
+  incoming(doEnable?: boolean): boolean {
+    return this._maybeEnableSound(Device.SoundName.Incoming, doEnable);
+  }
+
+  /**
+   * Enable or disable the outgoing sound.
+   * @param doEnable Passing `true` will enable the sound and `false` will disable the sound.
+   * Not passing this parameter will not alter the enable-status of the sound.
+   * @returns The enable-status of the sound.
+   */
+  outgoing(doEnable?: boolean): boolean {
+    return this._maybeEnableSound(Device.SoundName.Outgoing, doEnable);
+  }
+
   /**
    * Set the MediaTrackConstraints to be applied on every getUserMedia call for new input
    * device audio. Any deviceId specified here will be ignored. Instead, device IDs should
@@ -343,27 +378,6 @@ class AudioHelper extends EventEmitter {
     });
   }
 
-  /**
-   * Merge the passed enabledSounds into {@link AudioHelper}. Currently used to merge the deprecated
-   *   Device.sounds object onto the new {@link AudioHelper} interface. Mutates
-   *   by reference, sharing state between {@link Device} and {@link AudioHelper}.
-   * @param enabledSounds - The initial sound settings to merge.
-   * @private
-   */
-  private _addEnabledSounds(enabledSounds: { [name: string]: boolean }) {
-    function setValue(key: Device.ToggleableSound, value: boolean) {
-      if (typeof value !== 'undefined') {
-        enabledSounds[key] = value;
-      }
-
-      return enabledSounds[key];
-    }
-
-    Object.keys(enabledSounds).forEach(key => {
-      (this as any)[key] = setValue.bind(null, key);
-    });
-  }
-
   /**
    * Get the index of an un-labeled Device.
    * @param mediaDeviceInfo
@@ -407,6 +421,19 @@ class AudioHelper extends EventEmitter {
     });
   }
 
+  /**
+   * Set whether the sound is enabled or not
+   * @param soundName
+   * @param doEnable
+   * @returns Whether the sound is enabled or not
+   */
+  private _maybeEnableSound(soundName: Device.ToggleableSound, doEnable?: boolean): boolean {
+    if (typeof doEnable !== 'undefined') {
+      this._enabledSounds[soundName] = doEnable;
+    }
+    return this._enabledSounds[soundName];
+  }
+
   /**
    * Remove an input device from inputs
    * @param lostDevice
@@ -669,13 +696,6 @@ namespace AudioHelper {
      */
     audioContext?: AudioContext;
 
-    /**
-     * A Record of sounds. This is modified by reference, and is used to
-     * maintain backward-compatibility. This should be removed or refactored in 2.0.
-     * TODO: Remove / refactor in 2.0. (CLIENT-5302)
-     */
-    enabledSounds?: Record<Device.ToggleableSound, boolean>;
-
     /**
      * A custom MediaDevices instance to use.
      */

diff --git a/lib/twilio/device.ts b/lib/twilio/device.ts
@@ -354,15 +354,6 @@ class Device extends EventEmitter {
    */
   private _edge: string | null = null;
 
-  /**
-   * Whether each sound is enabled.
-   */
-  private _enabledSounds: Record<Device.ToggleableSound, boolean> = {
-    [Device.SoundName.Disconnect]: true,
-    [Device.SoundName.Incoming]: true,
-    [Device.SoundName.Outgoing]: true,
-  };
-
   /**
    * The name of the home region the {@link Device} is connected to.
    */
@@ -978,7 +969,7 @@ class Device extends EventEmitter {
       maxAverageBitrate: this._options.maxAverageBitrate,
       preflight: this._options.preflight,
       rtcConstraints: this._options.rtcConstraints,
-      shouldPlayDisconnect: () => this._enabledSounds.disconnect,
+      shouldPlayDisconnect: () => this.audio?.disconnect(),
       twimlParams,
       voiceEventSidGenerator: this._options.voiceEventSidGenerator,
     }, options);
@@ -1003,7 +994,7 @@ class Device extends EventEmitter {
         this._audio._maybeStartPollingVolume();
       }
 
-      if (call.direction === Call.CallDirection.Outgoing && this._enabledSounds.outgoing) {
+      if (call.direction === Call.CallDirection.Outgoing && this.audio?.outgoing()) {
         this._soundcache.get(Device.SoundName.Outgoing).play();
       }
 
@@ -1213,7 +1204,7 @@ class Device extends EventEmitter {
       this._publishNetworkChange();
     });
 
-    const play = (this._enabledSounds.incoming && !wasBusy)
+    const play = (this.audio?.incoming() && !wasBusy)
       ? () => this._soundcache.get(Device.SoundName.Incoming).play()
       : () => Promise.resolve();
 
@@ -1320,10 +1311,7 @@ class Device extends EventEmitter {
       this._updateSinkIds,
       this._updateInputStream,
       getUserMedia,
-      {
-        audioContext: Device.audioContext,
-        enabledSounds: this._enabledSounds,
-      },
+      { audioContext: Device.audioContext },
     );
 
     this._audio.on('deviceChange', (lostActiveDevices: MediaDeviceInfo[]) => {

diff --git a/package.json b/package.json
@@ -20,7 +20,7 @@
     "url": "[email protected]:twilio/twilio-voice.js.git"
   },
   "scripts": {
-    "build": "npm-run-all clean build:constants build:errors docs:ts build:es5 build:ts build:dist build:dist-min",
+    "build": "npm-run-all clean build:constants build:errors docs:ts build:es5 build:ts build:dist build:dist-min test:typecheck",
     "build:errors": "node ./scripts/errors.js",
     "build:es5": "rimraf ./es5 && babel lib -d es5",
     "build:dev": "ENV=dev npm run build",
@@ -55,6 +55,7 @@
     "test:integration": "karma start $PWD/karma.conf.ts",
     "test:network": "node ./scripts/karma.js $PWD/karma.network.conf.ts",
     "test:selenium": "mocha tests/browser/index.js",
+    "test:typecheck": "./node_modules/typescript/bin/tsc tests/typecheck/index.ts --noEmit",
     "test:unit": "nyc mocha -r ts-node/register ./tests/index.ts",
     "test:webpack": "cd ./tests/webpack && npm install && npm test"
   },

diff --git a/tests/audiohelper.js b/tests/audiohelper.js
@@ -297,7 +297,33 @@ describe('AudioHelper', () => {
       });
     });
 
-    describe('setAudioConstraints', () => {
+    ['disconnect', 'incoming', 'outgoing'].forEach(soundName => {
+      describe(`.${soundName}`, () => {
+        let testFn;
+
+        beforeEach(() => {
+          testFn = audio[soundName].bind(audio);
+        });
+
+        it('should return true as default', () => {
+          assert.strictEqual(testFn(), true);
+        });
+
+        it('should return false after setting to false', () => {
+          assert.strictEqual(testFn(false), false);
+          assert.strictEqual(testFn(), false);
+        });
+
+        it('should return true after setting to true', () => {
+          assert.strictEqual(testFn(false), false);
+          assert.strictEqual(testFn(), false);
+          assert.strictEqual(testFn(true), true);
+          assert.strictEqual(testFn(), true);
+        });
+      });
+    });
+
+    describe('.setAudioConstraints', () => {
       context('when no input device is active', () => {
         it('should set .audioConstraints', () => {
           audio.setAudioConstraints({ foo: 'bar' });
@@ -328,7 +354,7 @@ describe('AudioHelper', () => {
       });
     });
 
-    describe('unsetAudioConstraints', () => {
+    describe('.unsetAudioConstraints', () => {
       beforeEach(() => {
         audio.setAudioConstraints({ foo: 'bar' });
       });

diff --git a/tests/typecheck/index.ts b/tests/typecheck/index.ts
@@ -0,0 +1,15 @@
+import { Device } from '../../';
+
+(async () => {
+  const device: Device = new Device('foo', {});
+
+  await device.register();
+
+  const call = await device.connect({
+    params: { To: 'foo' }
+  });
+
+  device.audio?.disconnect(false);
+  device.audio?.incoming(false);
+  device.audio?.outgoing(false);
+});
diff --git a/tests/unit/device.ts b/tests/unit/device.ts
@@ -26,6 +26,7 @@ describe('Device', function() {
   let clock: SinonFakeTimers;
   let connectOptions: Record<string, any> | undefined;
   let device: Device;
+  let enabledSounds: Record<Device.ToggleableSound, boolean>;
   let pstream: any;
   let publisher: any;
   let stub: SinonStubbedInstance<Device>;
@@ -40,7 +41,11 @@ describe('Device', function() {
   const AudioHelper = (_updateSinkIds: Function, _updateInputStream: Function) => {
     updateInputStream = _updateInputStream;
     updateSinkIds = _updateSinkIds;
-    return audioHelper = createEmitterStub(require('../../lib/twilio/audiohelper').default);
+    const audioHelper = createEmitterStub(require('../../lib/twilio/audiohelper').default);
+    audioHelper.disconnect = () => enabledSounds[Device.SoundName.Disconnect];
+    audioHelper.incoming = () => enabledSounds[Device.SoundName.Incoming];
+    audioHelper.outgoing = () => enabledSounds[Device.SoundName.Outgoing];
+    return audioHelper;
   };
   const Call = (_?: any, _connectOptions?: Record<string, any>) => {
     connectOptions = _connectOptions;
@@ -63,6 +68,11 @@ describe('Device', function() {
   });
 
   beforeEach(() => {
+    enabledSounds = {
+      [Device.SoundName.Disconnect]: true,
+      [Device.SoundName.Incoming]: true,
+      [Device.SoundName.Outgoing]: true,
+    };
     pstream = null;
     publisher = null;
     clock = sinon.useFakeTimers(Date.now());
@@ -227,6 +237,16 @@ describe('Device', function() {
           activeCall.emit('accept');
           sinon.assert.calledOnce(spy.play);
         });
+
+        it('should not play outgoing sound after accepted if disabled', async () => {
+          enabledSounds[Device.SoundName.Outgoing] = false;
+          const spy: any = { play: sinon.spy() };
+          device['_soundcache'].set(Device.SoundName.Outgoing, spy);
+          await device.connect();
+          activeCall._direction = 'OUTGOING';
+          activeCall.emit('accept');
+          sinon.assert.notCalled(spy.play);
+        });
       });
 
       describe('.destroy()', () => {
@@ -670,14 +690,23 @@ describe('Device', function() {
           });
         });
 
-        it('should play the incoming sound', async () => {
+        it('should play the incoming sound if enabled', async () => {
           const spy = { play: sinon.spy() };
           device['_soundcache'].set(Device.SoundName.Incoming, spy);
           pstream.emit('invite', { callsid: 'foo', sdp: 'bar' });
           await clock.tickAsync(0);
           sinon.assert.calledOnce(spy.play);
         });
 
+        it('should not play the incoming sound if disabled', async () => {
+          enabledSounds[Device.SoundName.Incoming] = false;
+          const spy = { play: sinon.spy() };
+          device['_soundcache'].set(Device.SoundName.Incoming, spy);
+          pstream.emit('invite', { callsid: 'foo', sdp: 'bar' });
+          await clock.tickAsync(0);
+          sinon.assert.notCalled(spy.play);
+        });
+
         context('when allowIncomingWhileBusy is true', () => {
           beforeEach(async () => {
             device = new Device(token, { ...setupOptions, allowIncomingWhileBusy: true });