Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue-1473: App Crash with Invalid scriptURL #1476

Merged
merged 8 commits into from
May 15, 2024
Merged

Issue-1473: App Crash with Invalid scriptURL #1476

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4beeb2d
Use URL to parse URLs
morganick May 2, 2024
a4619bd
Adding NativeModule ref link
morganick May 2, 2024
ff96fe2
Make sure we also reset Nx cache
morganick May 3, 2024
b63fe4f
Using regex for host and falling back to defaultHost
morganick May 3, 2024
00571e1
Adding tests for getHostFromUrl functionality
morganick May 9, 2024
1fa045e
Add expo go URL scheme
morganick May 13, 2024
e74153f
Adding support for IPv6
morganick May 13, 2024
c3940d4
Removing `exp` scheme support - not needed
morganick May 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions lib/reactotron-react-native/src/reactotron-react-native.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,15 @@ let tempClientId: string | null = null
*
* On an Android emulator, if you want to connect any servers of local, you will need run adb reverse on your terminal. This function gets the localhost IP of host machine directly to bypass this.
*/
const getHost = (defaultHost = "localhost") =>
typeof NativeModules?.SourceCode?.getConstants().scriptURL === "string" // type guard in case this ever breaks https://github.com/facebook/react-native/blob/main/packages/react-native/Libraries/NativeModules/specs/NativeSourceCode.js#L15-L21
? NativeModules.SourceCode.scriptURL // Example: 'http://192.168.0.100:8081/index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=com.helloworld'
.split("://")[1] // Remove the scheme: '192.168.0.100:8081/index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=com.helloworld'
.split("/")[0] // Remove the path: '192.168.0.100:8081'
.split(":")[0] // Remove the port: '192.168.0.100'
: 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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return new URL(NativeModules?.SourceCode?.getConstants().scriptURL).hostname
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} catch (error) {
console.warn(`getHost: "${error.message}" for scriptURL - Falling back to ${defaultHost}`)
return defaultHost
}
}

const DEFAULTS: ClientOptions<ReactotronReactNative> = {
createSocket: (path: string) => new WebSocket(path), // eslint-disable-line
Expand Down