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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
71 changes: 71 additions & 0 deletions lib/reactotron-react-native/src/helpers/parseURL.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
import { getHostFromUrl } from "./parseURL"

describe("getHostFromUrl", () => {
it("should throw when no host is found", () => {
expect(() => {
getHostFromUrl("")
}).toThrow()
})

it("should get host from URL without scheme", () => {
Object.entries({
localhost: "localhost",
"127.0.0.1": "127.0.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to match on ipv6?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lol, this was the 🐰 🕳️ I was trying to avoid 😜 I have an idea for tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey, fair enough, you can tell me you don't think we need it! I have no idea whether we do or not.

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've added IPv6 and exp:// schema for Expo Go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

"[::1]": "[::1]",
}).forEach(([host, url]) => {
expect(getHostFromUrl(url)).toEqual(host)
})
expect(getHostFromUrl("localhost")).toEqual("localhost")
expect(getHostFromUrl("127.0.0.1")).toEqual("127.0.0.1")
})

it("should get the host from URL with http scheme", () => {
Object.entries({
localhost: "http://localhost",
"example.com": "http://example.com",
}).forEach(([host, url]) => {
expect(getHostFromUrl(url)).toEqual(host)
})
})

it("should get the host from URL with https scheme", () => {
Object.entries({
localhost: "https://localhost",
"example.com": "https://example.com",
}).forEach(([host, url]) => {
expect(getHostFromUrl(url)).toEqual(host)
})
})

it("should get the host from URL and ignore path, port, and query params", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also test with Expo tunnel-style URLs? https://docs.expo.dev/more/expo-cli/#tunneling.

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'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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It appears to work with Expo Go without it. Removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that makes sense 👍

Object.entries({
localhost:
"http://localhost:8081/.expo/.virtual-metro-entry.bundle?platform=ios&dev=true&lazy=true&minify=false&inlineSourceMap=false&modulesOnly=false&runModule=true&app=com.reactotronapp",
"192.168.1.141":
"https://192.168.1.141:8081/.expo/.virtual-metro-entry.bundle?platform=ios&dev=true&lazy=true&minify=false&inlineSourceMap=false&modulesOnly=false&runModule=true&app=com.reactotronapp",
}).forEach(([host, url]) => {
expect(getHostFromUrl(url)).toEqual(host)
})
})

it("should get the host from an IPv6 URL and ignore path, port, and query params", () => {
Object.entries({
"[::1]":
"http://[::1]:8081/.expo/.virtual-metro-entry.bundle?platform=ios&dev=true&lazy=true&minify=false&inlineSourceMap=false&modulesOnly=false&runModule=true&app=com.reactotronapp",
"[2001:0db8:85a3:0000:0000:8a2e:0370:7334]":
"https://[2001:0db8:85a3:0000:0000:8a2e:0370:7334]:8081/.expo/.virtual-metro-entry.bundle?platform=ios&dev=true&lazy=true&minify=false&inlineSourceMap=false&modulesOnly=false&runModule=true&app=com.reactotronapp",
}).forEach(([host, url]) => {
expect(getHostFromUrl(url)).toEqual(host)
})
})

it("should get the host from URL with hyphens", () => {
expect(getHostFromUrl("https://example-app.com")).toEqual("example-app.com")
})

it("should throw when the URL is an unsupported scheme", () => {
expect(() => {
getHostFromUrl("file:///Users/tron")
}).toThrow()
})
})
19 changes: 19 additions & 0 deletions lib/reactotron-react-native/src/helpers/parseURL.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/**
* Given a valid http(s) URL, the host for the given URL
* is returned.
*
* @param url {string} URL to extract the host from
* @returns {string} host of given URL or throws
*/
// Using a capture group to extract the hostname from a URL
export function getHostFromUrl(url: string) {
// Group 1: http(s)://
// Group 2: host
// Group 3: port
// Group 4: rest
const host = url.match(/^(?:https?:\/\/)?(\[[^\]]+\]|[^/:\s]+)(?::\d+)?(?:[/?#]|$)/)?.[1]

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

return host
}
20 changes: 13 additions & 7 deletions lib/reactotron-react-native/src/reactotron-react-native.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import networking, { NetworkingOptions } from "./plugins/networking"
import storybook from "./plugins/storybook"
import devTools from "./plugins/devTools"
import trackGlobalLogs from "./plugins/trackGlobalLogs"
import { getHostFromUrl } from "./helpers/parseURL"

const constants = NativeModules.PlatformConstants || {}

Expand All @@ -33,13 +34,18 @@ 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")

return getHostFromUrl(scriptURL)
} 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
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.

sh scripts/clean.sh

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