search

twilio-labs / dev-phone

A developer tool for testing SMS and Voice applications
MIT License
75 stars 20 forks source link

Improve SelectedPn variable usage in PhoneNumberPicker component #182

Closed hatealgebra closed 11 months ago

hatealgebra commented 12 months ago

Hi @ayyrickay,

Problem:

I've come across another bug when dev-phone-ui is run within the frontend env. The issue is not happening when the app is run with twilio dev-phone. and also it does not happen every time.

Here is the error in the console:

PhoneNumberPicker.jsx:48 Uncaught (in promise) ReferenceError: selectedPn is not defined
    at _callee$ (PhoneNumberPicker.jsx:48:19)
    at tryCatch (PhoneNumberPicker.jsx:2:1)
    at Generator.eval (PhoneNumberPicker.jsx:2:1)
    at Generator.eval [as next] (PhoneNumberPicker.jsx:2:1)
    at asyncGeneratorStep (PhoneNumberPicker.jsx:2:1)
    at _next (PhoneNumberPicker.jsx:2:1)
    at eval (PhoneNumberPicker.jsx:2:1)
    at new Promise (<anonymous>)
    at eval (PhoneNumberPicker.jsx:2:1)
    at numPicker (PhoneNumberPicker.jsx:48:16)

Here is the function which is causing the trouble (it is using the state from the component): https://github.com/twilio-labs/dev-phone/blob/ae08110f3d551b2fbd12c980268df7b2c65aa174/packages/dev-phone-ui/src/components/PhoneNumberPicker/PhoneNumberPicker.jsx#L49-L79

Solution:

I would either move this function underneath the states or pass the states/setStateActions as params.

Also IMO it would not be a bad idea to move all the smaller functions at least into the sibling file with the suffix *.util/helper.ts. I can see there is a similar issue for the backend: #43

Let me know what you think and I can make the changes after I will have permission for the push.

Edit 1: The issue is not happening every time, but most of the time. I use the latest Chrome on the latest MacOS.

ayyrickay commented 12 months ago

Good find. Looks like this is the same issue as what I encountered in #176.

I think this has always been one of the hardest parts of the code base to reason about. I agree that passing the selectedPn variable explicitly is a good idea here. That could also give us the opportunity to give the variable a clearer name that articulates what it's doing in relation to this function.