[wip] Update to use Twilio voice sdk 2.0#12
Conversation
enableRingingState and fakeLocalDtmf default to True in the 2.x versions of Twilio Voice SDK.
The Device object is now lazy about opening the signaling connection. `new Device(...)` is synchronous and will not cause a signaling channel to be opened, and the "ready" event won't be fired.
…bject. These events are now fired for Call objects instead of Device objects.
No functional change -- in the Twilio voice sdk 2.0, internal Connection objects were renamed to Call objects, so this is a semantic change to be in line with the internals.
These functions were previously not working because they did not have access to the device.
This is to disambiguate the payload that Twilio sends for inbound calls. The payload typically includes a "To" field already.
onlywade
left a comment
onlywade
left a comment
There was a problem hiding this comment.
Excellent work! Aside from the SDK upgrade, I dig the small improvements around comments, naming, and overall organization.
While I don't have much insight into the SDK, or Javascript best practices or style guidelines, etc, I can at least say that everything passes the sniff test as far as I'm concerned. Also, fwiw, the demo runs great using this branch and following the readme. 👍
| resp = VoiceResponse() | ||
| if "To" in request.form and request.form["To"] != '': | ||
| dial = Dial(caller_id=os.environ['TWILIO_CALLER_ID']) | ||
| if request.form.get("To") and request.form["To"] == twilio_number: |
There was a problem hiding this comment.
I presume that having two distinct conditional checks here may be intentional, for purposes of clarity. Personally, though, I would consider consolidating them into a single conditional, to be more concise:
if request.form.get("To") == twilio_number:
Just a minor preference - either way reads okay.
There was a problem hiding this comment.
Good call, the two checks were redundant 👍 I'll update.
stern-shawn
left a comment
stern-shawn
left a comment
There was a problem hiding this comment.
Love this and the detailed readme changes!
I can't speak to the usage of the Twilio client itself, but I have some small JS-specific nits to toss into the ring 😆
|
|
||
| speakerDevices.addEventListener("change", function () { | ||
| var selectedDevices = [].slice | ||
| const selectedDevices = [].slice |
There was a problem hiding this comment.
Looks like this got dredged up by the var -> const change, haha.
[].slice.call is a pretty esoteric way to convert a NodeList into an Array. Would recommend using the more readable/beginner-friendly Array.from method. MDN link
| const selectedDevices = [].slice | |
| const selectedDevices = Array.from(speakerDevices.children) | |
| .filter((node) => node.selected); // the filter function can also be shortened :) |
|
|
||
| ringtoneDevices.addEventListener("change", function () { | ||
| var selectedDevices = [].slice | ||
| const selectedDevices = [].slice |
There was a problem hiding this comment.
Same as the other comment on [].slice.call
Remove example of creating a new API key via the twilio cli, because doing so requires retrieving the account's auth token. Also, make a note that the API_KEY is the key's SID.
Brianna made structure changes to the frontend code to make it more readable/understandable. https://github.com/TwilioDevEd/voice-javascript-sdk-quickstart-node This also lints the frontend code with prettier.js.
3 participants
No description provided.