-
Notifications
You must be signed in to change notification settings - Fork 943
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
Changes from 5 commits
4beeb2d
a4619bd
ff96fe2
b63fe4f
00571e1
1fa045e
e74153f
c3940d4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
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", | ||
}).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", () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It appears to work with Expo Go without it. Removed. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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() | ||
}) | ||
}) |
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 | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 || {} | ||
|
||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
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 | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 runexpo start
it says that metro is waiting onexp://192.168.1.x:8081
so if that comes through on thescriptURL
we'll handle it.I added specs for both IPv6 and
exp
schema.