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

Show file tree
Hide file tree
Changes from 4 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
24 changes: 17 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,23 @@ 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.

const scriptURL = NativeModules?.SourceCode?.getConstants().scriptURL
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]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test cases: https://regex101.com/r/WIHVuQ/2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 😄


if (typeof host !== "string") throw new Error("Invalid URL - host not found")

return host
} 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
3 changes: 3 additions & 0 deletions scripts/reset.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joshuayoes https://nx.dev/nx-api/nx/documents/reset

sh scripts/clean.sh

echo "Removing all node_modules folders from the project"
Expand Down