search

twilio / twilio-voice.js

Twilio's JavaScript Voice SDK
Other
50 stars 53 forks source link

Update Error class to give more specific errors #3

Open Lyra-B opened 3 years ago

Lyra-B commented 3 years ago

My team is using twilio-client.js in our telephony app and we are experiencing the following issue. When a twilio networking error occurs we do not have enough information to handle it properly. The code that is relevant to this issue is here. This error is too generic for us to handle specifically and we are looking for advice on how to move forward.

We noticed  that twilio-voice.js is the next version for this library and according to this it contains much more detailed information about errors, however this particular error class remains as generic as in twilio-client.js

ryan-rowland commented 3 years ago

Hi Lyra, I apologize for the delayed response here and thanks for submitting this issue. In this case, we are passing back the exact error that we get from the request. The line you linked to could be getting called from multiple places; could you go into more detail on the error(s) you're receiving and how you're receiving them?

doryphores commented 3 years ago

The issue here is that we use Airbrake to report JavaScript errors. The airbrake JS SDK registers a global unhandledrejection listener that reports all uncaught promise rejections. This is resulting in any error coming from this request module being reported to our airbrake account because of the following:

The request module in question is used by EventPublisher which returns a promise that is rejected when the xhr request fails. Neither EventPublisher or Call which uses it handles the rejection so these errors are being caught by airbrake's unhandledrejection listener. The errors are simple Error classes so we can't even filter them out (if they were TwilioError instances with a specific code, we could do this).

I'm not sure what the right approach is here. I can see why you may not want to wrap these as TwilioError as they're xhr request errors. But you may want to stop rejecting the promise when they fail (delete this line?). The error is emitted as an error event anyway and I can see that these are being logged as a warning: https://github.com/twilio/twilio-voice.js/blob/385c31fc7a2492f7b4f279af3816aa36365597b3/lib/twilio/device.ts#L1252-L1255

ryan-rowland commented 3 years ago

Thanks @doryphores, I see what you mean. I agree that we should either catch these rejections in the SDK (if we plan to do something about them) or change this method to return void, as you've suggested. I will create an internal ticket for this, and I'll update this ticket once we've addressed the issue.