-
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 1 commit
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 | ||
} |
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.