Skip to content
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

VBLOCKS-981 VBLOCKS-1093 | Updating eventing API per latest specs #116

Merged
merged 2 commits into from
Oct 5, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,4 @@ extension/token.js
node_modules
docs
lib/twilio/constants.js
nodemon.json
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm running nodemon locally to watch and auto build the SDK. I didn't want to commit my personal config.

154 changes: 116 additions & 38 deletions lib/twilio/call.ts
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,12 @@ class Call extends EventEmitter {
*/
private _mediaStatus: Call.State = Call.State.Pending;

/**
* A map of messages sent via sendMessage API.
* The message will be deleted once an 'ack' or an error is received from the server.
*/
private _messages: Map<string, Call.Message> = new Map();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming the string is the voiceEventSid of the message? Should we write that down?


/**
* A batch of metrics samples to send to Insights. Gets cleared after
* each send and appended to on each new sample.
Expand Down Expand Up @@ -524,11 +530,13 @@ class Call extends EventEmitter {
};

this._pstream = config.pstream;
this._pstream.on('ack', this._onAck);
this._pstream.on('cancel', this._onCancel);
this._pstream.on('error', this._onSignalingError);
this._pstream.on('ringing', this._onRinging);
this._pstream.on('transportClose', this._onTransportClose);
this._pstream.on('connected', this._onConnected);
this._pstream.on('message', this._onMessage);
this._pstream.on('message', this._onMessageReceived);

this.on('error', error => {
this._publisher.error('connection', 'error', {
Expand Down Expand Up @@ -576,14 +584,6 @@ class Call extends EventEmitter {
const rtcConfiguration = options.rtcConfiguration || this._options.rtcConfiguration;
const rtcConstraints = options.rtcConstraints || this._options.rtcConstraints || { };
const audioConstraints = rtcConstraints.audio || { audio: true };
const messagesToRegisterFor = options.messagesToRegisterFor || [];

if (
!Array.isArray(messagesToRegisterFor) ||
messagesToRegisterFor.some((event) => typeof event !== 'string')
) {
throw new Error('`messagesToRegisterFor` option must be an array of strings.');
}

this._status = Call.State.Connecting;

Expand Down Expand Up @@ -638,7 +638,7 @@ class Call extends EventEmitter {
`${encodeURIComponent(pair[0])}=${encodeURIComponent(pair[1])}`).join('&');
this._pstream.on('answer', this._onAnswer.bind(this));
this._mediaHandler.makeOutgoingCall(this._pstream.token, params, this.outboundConnectionId,
rtcConstraints, rtcConfiguration, messagesToRegisterFor, onAnswer);
rtcConstraints, rtcConfiguration, onAnswer);
}
};

Expand Down Expand Up @@ -863,19 +863,15 @@ class Call extends EventEmitter {
}

/**
* Send a message to Twilio.
*
* @example
* ```ts
* call.sendMessage(Call.MessageType.UserDefinedMessage, {
* ahoy: 'world!,
* });
* ```
*
* @param messageType - The type of the message to send to Twilio.
* @param content - The content of the message to send to Twilio.
* TODO
*/
sendMessage(messageType: Call.MessageType, content: any) {
sendMessage(message: Call.Message): string {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does returning a Promise here that resolves when we receive ack make sense?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We initially thought of returning a promise once ack is received. However, it's not the pattern that is used in our SDK. We use pub-sub style for every event that we receive from signaling. In this case, we emit messageSent once we receive the ack.

const { content, contentType, messageType } = message;

if (typeof content === 'undefined' || content === null) {
throw new InvalidArgumentError('`content` is empty');
}

if (typeof messageType !== 'string') {
throw new InvalidArgumentError(
'`messageType` must be an enumeration value of `Call.MessageType` or ' +
Expand Down Expand Up @@ -903,7 +899,9 @@ class Call extends EventEmitter {
}

const voiceEventSid = this._voiceEventSidGenerator();
this._pstream.sendMessage(callSid, voiceEventSid, messageType, content);
this._messages.set(voiceEventSid, { content, contentType, messageType, voiceEventSid });
this._pstream.sendMessage(callSid, content, contentType, messageType, voiceEventSid);
return voiceEventSid;
}

/**
Expand Down Expand Up @@ -954,13 +952,15 @@ class Call extends EventEmitter {
const cleanup = () => {
if (!this._pstream) { return; }

this._pstream.removeListener('ack', this._onAck);
this._pstream.removeListener('answer', this._onAnswer);
this._pstream.removeListener('cancel', this._onCancel);
this._pstream.removeListener('error', this._onSignalingError);
this._pstream.removeListener('hangup', this._onHangup);
this._pstream.removeListener('ringing', this._onRinging);
this._pstream.removeListener('transportClose', this._onTransportClose);
this._pstream.removeListener('connected', this._onConnected);
this._pstream.removeListener('message', this._onMessage);
this._pstream.removeListener('message', this._onMessageReceived);
};

// This is kind of a hack, but it lets us avoid rewriting more code.
Expand Down Expand Up @@ -1094,6 +1094,21 @@ class Call extends EventEmitter {
}
}

/**
* Called when the {@link Call} receives an ack from signaling
* @param payload
*/
private _onAck = (payload: Record<string, any>): void => {
Copy link
Collaborator

@mhuynh5757 mhuynh5757 Oct 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about _onMessageAck for clarity and to match _onMessageReceived?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'ack' is not specific to messages. It's for any VSP event.
https://code.hq.twilio.com/twilio/voice-signaling-protocol#event-ack

const { acktype, callsid, voiceeventsid } = payload;
if (this.parameters.CallSid !== callsid) {
this._log.warn(`Received ack from a different callsid: ${callsid}`);
return;
}
if (acktype === 'message') {
this._onMessageSent(voiceeventsid);
}
}

/**
* Called when the {@link Call} is answered.
* @param payload
Expand Down Expand Up @@ -1280,7 +1295,7 @@ class Call extends EventEmitter {

/**
* Raised when a Call receives a message from the backend.
*
* TODO
* @remarks
* Note that in this context a "message" is limited to a
* "User Defined Message" from the Voice User Defined Message Service (VUDMS)
Expand All @@ -1290,12 +1305,34 @@ class Call extends EventEmitter {
* @param payload - A record representing the payload of the message from the
* Twilio backend.
*/
private _onMessage = (payload: Record<string, string>): void => {
if (this.parameters.CallSid !== payload.callsid) {
private _onMessageReceived = (payload: Record<string, any>): void => {
const { callsid, content, contenttype, messagetype, voiceeventsid } = payload;

if (this.parameters.CallSid !== callsid) {
this._log.warn(`Received a message from a different callsid: ${callsid}`);
return;
}

this.emit('message', payload);
this.emit('messageReceived', {
content,
contentType: contenttype,
messageType: messagetype,
voiceEventSid: voiceeventsid,
});
}

/**
* TODO
* @param voiceEventSid
*/
private _onMessageSent = (voiceEventSid: string): void => {
if (!this._messages.has(voiceEventSid)) {
this._log.warn(`Received a messageSent with a voiceEventSid that doesn't exists: ${voiceEventSid}`);
return;
}
const message = this._messages.get(voiceEventSid);
this._messages.delete(voiceEventSid);
this.emit('messageSent', message);
}

/**
Expand Down Expand Up @@ -1338,6 +1375,22 @@ class Call extends EventEmitter {
this.emit('sample', sample);
}

/**
* Called when an 'error' event is received from the signaling stream.
*/
private _onSignalingError = (payload: Record<string, any>): void => {
const { callsid, voiceeventsid } = payload;
if (this.parameters.CallSid !== callsid) {
this._log.warn(`Received an error from a different callsid: ${callsid}`);
return;
}
if (voiceeventsid && this._messages.has(voiceeventsid)) {
// Do not emit an error here. Device is handling all signaling related errors.
this._messages.delete(voiceeventsid);
this._log.warn(`Received an error while sending a message.`, payload);
}
}

/**
* Called when signaling is restored
*/
Expand Down Expand Up @@ -1480,7 +1533,7 @@ namespace Call {

/**
* Emitted when a Call receives a message from the backend.
*
* TODO
* @remarks
* Note that in this context a "message" is limited to a
* "User Defined Message" from the Voice User Defined Message Service (VUDMS)
Expand All @@ -1492,7 +1545,12 @@ namespace Call {
* @example `call.on('message', (payload) => { })`
* @event
*/
declare function messageEvent(payload: Record<string, string>): void;
declare function messageReceivedEvent(message: Call.Message): void;

/**
* TODO
*/
declare function messageSentEvent(message: Call.Message): void;

/**
* Emitted when the {@link Call} is muted or unmuted.
Expand Down Expand Up @@ -1626,14 +1684,6 @@ namespace Call {
* Options to be used to acquire media tracks and connect media.
*/
export interface AcceptOptions {
/**
* An array containing strings representing events that this call is
* registering for. For example, "dial-callprogress-events" so that this
* call will raise such events to the end-user when the stream receives
* them.
*/
messagesToRegisterFor?: Call.MessageType[];

/**
* An RTCConfiguration to pass to the RTCPeerConnection constructor.
*/
Expand Down Expand Up @@ -1699,6 +1749,34 @@ namespace Call {
soundcache: Map<Device.SoundName, ISound>;
}

/**
* A Call Message represents the data that is being transferred between
* the signaling server and the SDK.
*/
export interface Message {
/**
* The content of the message which should match the contentType parameter.
*/
content: any;

/**
* The MIME type of the content. Default is application/json.
*/
contentType?: string;

/**
* The type of message
*/
messageType: MessageType;

/**
* An autogenerated id that uniquely identifies the instance of this message.
* This is not required when sending a message from the SDK as this is autogenerated.
* But it will be available after the message is sent, or when a message is received.
*/
voiceEventSid?: string;
}

/**
* Options to be passed to the {@link Call} constructor.
* @private
Expand Down
10 changes: 1 addition & 9 deletions lib/twilio/device.ts
Original file line number Diff line number Diff line change
Expand Up @@ -564,14 +564,6 @@ class Device extends EventEmitter {
throw new InvalidStateError('A Call is already active');
}

const messagesToRegisterFor = options.messagesToRegisterFor || [];
if (
!Array.isArray(messagesToRegisterFor) ||
messagesToRegisterFor.some((event) => typeof event !== 'string')
) {
throw new InvalidArgumentError('`messagesToRegisterFor` option must be an array of strings.');
}

const activeCall = this._activeCall = await this._makeCall(options.params || { }, {
rtcConfiguration: options.rtcConfiguration,
voiceEventSidGenerator: this._options.voiceEventSidGenerator,
Expand All @@ -583,7 +575,7 @@ class Device extends EventEmitter {
// Stop the incoming sound if it's playing
this._soundcache.get(Device.SoundName.Incoming).stop();

activeCall.accept({ rtcConstraints: options.rtcConstraints, messagesToRegisterFor });
activeCall.accept({ rtcConstraints: options.rtcConstraints });
this._publishNetworkChange();
return activeCall;
}
Expand Down
20 changes: 10 additions & 10 deletions lib/twilio/pstream.js
Original file line number Diff line number Diff line change
Expand Up @@ -178,17 +178,18 @@ PStream.prototype.setToken = function(token) {
};

PStream.prototype.sendMessage = function(
callSid,
voiceEventSid,
messageType,
content
callsid,
content,
contenttype = 'application/json',
messagetype,
voiceeventsid
) {
const payload = {
contenttype: 'application/json',
messagetype: messageType,
callsid,
content,
callsid: callSid,
voiceeventsid: voiceEventSid,
contenttype,
messagetype,
voiceeventsid,
};
this._publish('message', payload, true);
};
Expand All @@ -200,12 +201,11 @@ PStream.prototype.register = function(mediaCapabilities) {
this._publish('register', regPayload, true);
};

PStream.prototype.invite = function(sdp, callsid, preflight, params, registerFor) {
PStream.prototype.invite = function(sdp, callsid, preflight, params) {
const payload = {
callsid,
sdp,
preflight: !!preflight,
registerFor,
twilio: params ? { params } : {}
};
this._publish('invite', payload, true);
Expand Down
18 changes: 2 additions & 16 deletions lib/twilio/rtc/peerconnection.js
Original file line number Diff line number Diff line change
Expand Up @@ -852,15 +852,7 @@ PeerConnection.prototype.iceRestart = function() {
});
};

PeerConnection.prototype.makeOutgoingCall = function(
token,
params,
callsid,
rtcConstraints,
rtcConfiguration,
messagesToRegisterFor,
onMediaStarted
) {
PeerConnection.prototype.makeOutgoingCall = function(token, params, callsid, rtcConstraints, rtcConfiguration, onMediaStarted) {
if (!this._initializeMediaStream(rtcConstraints, rtcConfiguration)) {
return;
}
Expand Down Expand Up @@ -897,13 +889,7 @@ PeerConnection.prototype.makeOutgoingCall = function(

function onOfferSuccess() {
if (self.status !== 'closed') {
self.pstream.invite(
self.version.getSDP(),
self.callSid,
self.options.preflight,
params,
messagesToRegisterFor
);
self.pstream.invite(self.version.getSDP(), self.callSid, self.options.preflight, params);
self._setupRTCDtlsTransportListener();
}
}
Expand Down
2 changes: 1 addition & 1 deletion lib/twilio/uuid.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ function generateUuid(): string {

const generateRandomValues: () => string =
typeof crypto.randomUUID === 'function'
? crypto.randomUUID
? () => crypto.randomUUID!()
Copy link
Collaborator

@mhuynh5757 mhuynh5757 Oct 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this where the illegal invocation was occurring? You could also do

? crypto.randomUUID?.bind(crypto)

but it doesn't make too much of a difference.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this was the root cause of illegal invocation. I used an arrow function instead of function binding to make it consistent with the line below it.

: () => crypto.getRandomValues(new Uint32Array(32)).toString();

return md5(generateRandomValues());
Expand Down
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -122,10 +122,10 @@
"dependencies": {
"@twilio/audioplayer": "1.0.6",
"@twilio/voice-errors": "1.1.1",
"@types/md5": "^2.3.2",
"@types/md5": "2.3.2",
"backoff": "2.5.0",
"loglevel": "1.6.7",
"md5": "^2.3.0",
"md5": "2.3.0",
"rtcpeerconnection-shim": "1.2.8",
"ws": "7.4.6",
"xmlhttprequest": "1.8.0"
Expand Down
Loading