reactotron icon indicating copy to clipboard operation
reactotron copied to clipboard

Issue-1473: App Crash with Invalid scriptURL

Open morganick opened this issue 1 year ago • 3 comments

Please verify the following:

  • [x] yarn build-and-test:local passes
  • [x] I have added tests for any new features, if relevant
  • [ ] README.md (or relevant documentation) has been updated with your changes

Describe your PR

Fixes #1473 where calling split on an invalid string throws a TypeError.

In the event that the following line does not provide us with a valid URL: https://github.com/facebook/react-native/blob/b38f80aeb6ad986c64fd03f53b2e01a7990e1533/packages/react-native/React/CoreModules/RCTSourceCode.mm#L38

We should catch this and warn the user that we are falling back to the default host:

WARN  getHost: "Invalid URL: null" for scriptURL - Falling back to localhost

How to Reproduce

I was unable to reproduce this issue locally with my devices as it would not crash the application, but just throw the error. Due to the circumstances of the original issue and that applications configuration, this should allow that application to continue to boot.

Additional Notes

It is still possible to configure Reactotron with an invalid host. This will equally throw an error of invalid url. I'd like to create a new PR that allows the application to continue to function, keeps Reactotron is a disconnected state, and notifies the user.

morganick avatar May 02 '24 15:05 morganick

@lindboe I have validated this with Expo apps on my devices, simulator, and emulator for both iOS and Android. I also validated this on device Android and iOS simulator with RN 0.74 app using RN cli.

Could you take this for a spin inside your application on your devices to make sure we've covered the cases we need to.

morganick avatar May 03 '24 19:05 morganick

Hey guys, any news on when this will be released?

migueldaipre avatar May 08 '24 14:05 migueldaipre

@joshuayoes I went full regex based on our conversation on Friday.

morganick avatar May 13 '24 20:05 morganick