Issue-1473: App Crash with Invalid scriptURL #1476
Conversation
| const getHost = (defaultHost = "localhost") => { | ||
| try { | ||
| // RN Reference: https://github.com/facebook/react-native/blob/main/packages/react-native/src/private/specs/modules/NativeSourceCode.js | ||
| return new URL(NativeModules?.SourceCode?.getConstants().scriptURL).hostname |
There was a problem hiding this comment.
Didn't see any reason not to use URL class here for this purpose:
https://github.com/facebook/react-native/blob/b38f80aeb6ad986c64fd03f53b2e01a7990e1533/packages/react-native/types/modules/globals.d.ts#L396
There was a problem hiding this comment.
IIRC Hermes URL implementation has some problems. Josh has commented on this: facebook/hermes#1072 (comment). I'm not sure of the current state, but looking...
From https://www.npmjs.com/package/react-native-url-polyfill, they claim it doesn't work with 'localhost' properly (facebook/react-native#26019), and creating new URLs can throw exceptions (facebook/react-native#16434).
I think if we take this route we need to be sure to test it with a non-Expo app, since as that discussion mentions they added their own polyfill. Maybe test the oldest supported RN version too in case changes have been made that I haven't seen.
There was a problem hiding this comment.
Looks like the Expo polyfill was doing its job. The above screenshot is from a fresh react native cli app. I'll switch it out with something that handles just this case.
There was a problem hiding this comment.
I was unable to use the same package that Expo is using for their URL Here's the scoop:
We use rollup for building the core dependencies into the packages like reactotron-react-native. I was trying to use a package that uses commonjs exports inside of the library. Evidently, Rollup has deprecated the way that is handled and moved it into another package: https://rollupjs.org/troubleshooting/#error-name-is-not-exported-by-module ( I was specifically running into this issue)
I could install that package in any other application, but not for use in our monorepo because of rollup... but I didn't figure that out until this morning because after I removed it... reactotron-react-native would no longer build. Even after reverting all of the changes calling yarn clean and yarn reset. It wasn't until I switched branches back to master... where it started to build again. Then I switched back to my branch without the new package and it continued the build 😱
So I was then trying to figure out what cache is not being cleared. (I didn't realize rollup was involved.) Come to find out that I believe the nx cache is cleared when you switch branches and code changes in that dep. So in order for me to reliably add that package and not foobar the build I would have to run yarn nx reset && yarn reset.
I have updated our yarn reset script for this reason to include a call to yarn nx reset.
I moved this to being a regex for now until we can do the prerequisite Rollup upgrades.
| : defaultHost | ||
| const getHost = (defaultHost = "localhost") => { | ||
| try { | ||
| // RN Reference: https://github.com/facebook/react-native/blob/main/packages/react-native/src/private/specs/modules/NativeSourceCode.js |
There was a problem hiding this comment.
This URL gives you more context as to what's available with this native module. I can change it back to the old link if that's more clear.
| if (typeof scriptURL !== "string") throw new Error("Invalid non-string URL") | ||
|
|
||
| // Using a capture group to extract the hostname from a URL | ||
| const host = scriptURL.match(/^(?:https?:\/\/)?([^/:\s]+)(?::\d+)?(?:[/?#]|$)/)?.[1] |
There was a problem hiding this comment.
Test cases: https://regex101.com/r/WIHVuQ/2
There was a problem hiding this comment.
Future 🤔 ...
We could try to use the built in URL class if it has the host or hostname implemented. This would work in current Expo applications. We could then fall back to the regex if that's not available.
Reasons:
- Using what's there so it stays in line with what the application is already using
- More robust support for URLs - The regex above does not support IPv6.
- Hope that URL parsing gets fully implemented so that it's available across the board without polyfill or external packages and then we can remove the regex.
For now, we'll utilize the regex until we can either import the standardized URL parser or it becomes a first class citizen of the platform. To @lindboe point, having one solution means the potential bug reports will be simpler to triage.
There was a problem hiding this comment.
Could we put these regex in the codebase as unit tests? That way we can add more tests if we have more URL issues in the future.
There was a problem hiding this comment.
One thing I like to do with regexes is comments that explain their matching section-by-section (here it seems feasible to do per-capture group) just to make sure everybody's on the same page about what's intended
Unit tests aren't a bad idea either 😄
| @@ -1,5 +1,8 @@ | |||
| #!/bin/bash | |||
|
|
|||
| echo "Nx Reset - Clears all the cached Nx artifacts and metadata about the workspace and shuts down the Nx Daemon." | |||
| yarn nx reset | |||
|
|
|||
There was a problem hiding this comment.
|
@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. |
|
Hey guys, any news on when this will be released? |
| it("should get host from URL without scheme", () => { | ||
| Object.entries({ | ||
| localhost: "localhost", | ||
| "127.0.0.1": "127.0.0.1", |
There was a problem hiding this comment.
do we need to match on ipv6?
There was a problem hiding this comment.
Lol, this was the 🐰 🕳️ I was trying to avoid 😜 I have an idea for tomorrow.
There was a problem hiding this comment.
Hey, fair enough, you can tell me you don't think we need it! I have no idea whether we do or not.
There was a problem hiding this comment.
I've added IPv6 and exp:// schema for Expo Go.
There was a problem hiding this comment.
exp:// may not be needed, but I included it as when you run expo start it says that metro is waiting on exp://192.168.1.x:8081 so if that comes through on the scriptURL we'll handle it.
I added specs for both IPv6 and exp schema.
| }) | ||
| }) | ||
|
|
||
| it("should get the host from URL and ignore path, port, and query params", () => { |
There was a problem hiding this comment.
Should we also test with Expo tunnel-style URLs? https://docs.expo.dev/more/expo-cli/#tunneling.
There was a problem hiding this comment.
I'm not quite sure how to support tunneling honestly. I think you'd have to create a tunnel for each port that you wanted to connect to. This is something I'd probably reach for Tailscale for... which reminds me I should create a blog post for that. Either way, I think we'd still get the host right for it. Just would have to make sure we forwarded the right ports to Reactotron, yes?
There was a problem hiding this comment.
With this comment I only meant we should check we don't break anything that wasn't broken for Expo's tunneling support. I'm not sure if exp:// is used there for hosting the JS bundle, I thought maybe it was just used for deep links now? Not sure.
There was a problem hiding this comment.
It appears to work with Expo Go without it. Removed.
There was a problem hiding this comment.
@lindboe In order to support tunneling, I believe we'd have to have some way to know to include the port. This PR only replaces the old method of finding the host with safeguards around not crashing the application if the URL is not valid. Maybe we tackle that in another PR?
|
@joshuayoes I went full regex based on our conversation on Friday. |
Please verify the following:
yarn build-and-test:localpassesREADME.md(or relevant documentation) has been updated with your changesDescribe 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:
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.