twilio-node icon indicating copy to clipboard operation
twilio-node copied to clipboard

Errors thrown in each callback are not printed to stderr

Open jsakas opened this issue 3 years ago • 3 comments
trafficstars

Issue Summary

I am finding that done is never called in client.incomingPhoneNumbers.each.

I believe this could be the same issue originally reported in https://github.com/twilio/twilio-node/issues/678.

Please see my code snippet below.

Code Snippet

async function getActivePhoneNumbers(limit: number): Promise<IncomingPhoneNumberInstance[]> {
  return new Promise((resolve) => {
    let numbers: IncomingPhoneNumberInstance[];

    twilioClient.incomingPhoneNumbers.each({
      limit,
      done: () => {
        resolve(numbers)
      },
      callback: (number) => {
        numbers.push(number)
      }
    })
  })
}

await getActivePhoneNumbers(3)

Exception/Log

# paste exception/log here

Technical details:

  • twilio-node version: ^3.80.1
  • node version: v14.17.6

jsakas avatar Aug 24 '22 06:08 jsakas

Ok - after linking the twilio module locally to try to resolve the bug myself, I found that done is in-fact called correctly, but any errors within the callback are not printed to stderr.

The issue is here:

let numbers: IncomingPhoneNumberInstance[];

numbers.push(number);            // TypeError: Cannot read property 'push' of undefined

Perhaps I shouldn't work so late at night.

Regardless, I am surprised the TypeScript compiler did not warn me of this, and the error was swallowed due to the binding/recursive technique used in the function.

I would suggest wrapping the call to callback in a try / catch with explicit console.error logging so users are informed of any errors, and then throw the error to stop execution as expected.

jsakas avatar Aug 25 '22 04:08 jsakas

It looks like since these are generated files I can't submit a PR but this is what I added locally: https://github.com/twilio/twilio-node/compare/main...jsakas:twilio-node:main

jsakas avatar Aug 25 '22 04:08 jsakas

I was able to recreate this locally and agree that this is not something that should get swallowed, though I don't agree logging the error is the best solution (though it may be given the current implementation).

We're currently working on a refactor of the generated code and I've added this issue to the acceptance criteria for building out the pagination code (ref: DI-2415).

childish-sambino avatar Aug 30 '22 20:08 childish-sambino

Fixed by https://github.com/twilio/twilio-node/pull/869

childish-sambino avatar Jan 17 '23 16:01 childish-sambino