-
-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Fix URL creation with memory history #9814
Changes from 3 commits
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,5 @@ | ||
--- | ||
"@remix-run/router": patch | ||
--- | ||
|
||
Fix URL creation with memory histories |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,107 @@ | ||
/** | ||
* @jest-environment node | ||
*/ | ||
Comment on lines
+1
to
+3
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. Because tests by default run in jsdom, we didn't catch this regression since |
||
|
||
import { createMemoryHistory, createRouter } from "../index"; | ||
|
||
// This suite of tests specifically runs in the node jest environment to catch | ||
// issues when window is not present | ||
describe("a memory router", () => { | ||
it("initializes properly", async () => { | ||
let router = createRouter({ | ||
routes: [ | ||
{ | ||
path: "/", | ||
}, | ||
], | ||
history: createMemoryHistory(), | ||
}); | ||
expect(router.state).toEqual({ | ||
historyAction: "POP", | ||
loaderData: {}, | ||
actionData: null, | ||
errors: null, | ||
location: { | ||
hash: "", | ||
key: expect.any(String), | ||
pathname: "/", | ||
search: "", | ||
state: null, | ||
}, | ||
matches: [ | ||
{ | ||
params: {}, | ||
pathname: "/", | ||
pathnameBase: "/", | ||
route: { | ||
id: "0", | ||
path: "/", | ||
}, | ||
}, | ||
], | ||
initialized: true, | ||
navigation: { | ||
location: undefined, | ||
state: "idle", | ||
}, | ||
preventScrollReset: false, | ||
restoreScrollPosition: null, | ||
revalidation: "idle", | ||
fetchers: new Map(), | ||
}); | ||
router.dispose(); | ||
}); | ||
|
||
it("can create Requests without window", async () => { | ||
let loaderSpy = jest.fn(); | ||
let router = createRouter({ | ||
routes: [ | ||
{ | ||
path: "/", | ||
}, | ||
{ | ||
path: "/a", | ||
loader: loaderSpy, | ||
}, | ||
], | ||
history: createMemoryHistory(), | ||
}); | ||
|
||
router.navigate("/a"); | ||
expect(loaderSpy.mock.calls[0][0].request.url).toBe("http://localhost/a"); | ||
router.dispose(); | ||
}); | ||
|
||
it("can create URLs without window", async () => { | ||
let shouldRevalidateSpy = jest.fn(); | ||
|
||
let router = createRouter({ | ||
routes: [ | ||
{ | ||
path: "/", | ||
loader: () => "ROOT", | ||
shouldRevalidate: shouldRevalidateSpy, | ||
children: [ | ||
{ | ||
index: true, | ||
}, | ||
{ | ||
path: "a", | ||
}, | ||
], | ||
}, | ||
], | ||
history: createMemoryHistory(), | ||
hydrationData: { loaderData: { "0": "ROOT" } }, | ||
}); | ||
|
||
router.navigate("/a"); | ||
expect(shouldRevalidateSpy.mock.calls[0][0].currentUrl.toString()).toBe( | ||
"http://localhost/" | ||
); | ||
expect(shouldRevalidateSpy.mock.calls[0][0].nextUrl.toString()).toBe( | ||
"http://localhost/a" | ||
); | ||
router.dispose(); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,9 @@ | ||
import { | ||
TextEncoder as NodeTextEncoder, | ||
TextDecoder as NodeTextDecoder, | ||
} from "util"; | ||
import { fetch, Request, Response } from "@remix-run/web-fetch"; | ||
import { AbortController as NodeAbortController } from "abort-controller"; | ||
|
||
if (!globalThis.fetch) { | ||
// Built-in lib.dom.d.ts expects `fetch(Request | string, ...)` but the web | ||
|
@@ -13,3 +18,14 @@ if (!globalThis.fetch) { | |
// @ts-expect-error | ||
globalThis.Response = Response; | ||
} | ||
|
||
if (!globalThis.AbortController) { | ||
// @ts-expect-error | ||
globalThis.AbortController = NodeAbortController; | ||
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. Needed for the new node test run |
||
} | ||
|
||
if (!globalThis.TextEncoder || !globalThis.TextDecoder) { | ||
globalThis.TextEncoder = NodeTextEncoder; | ||
// @ts-expect-error | ||
globalThis.TextDecoder = NodeTextDecoder; | ||
} | ||
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. Copied over from the deleted custom environment |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -125,6 +125,13 @@ export interface History { | |
*/ | ||
createHref(to: To): string; | ||
|
||
/** | ||
* Returns a URL for the given `to` value | ||
* | ||
* @param to - The destination URL | ||
*/ | ||
createURL(to: To): URL; | ||
|
||
/** | ||
* Encode a location the same way window.history would do (no-op for memory | ||
* history) so we ensure our PUSH/REPLACE navigations for data routers | ||
|
@@ -255,6 +262,10 @@ export function createMemoryHistory( | |
return location; | ||
} | ||
|
||
function createHref(to: To) { | ||
return typeof to === "string" ? to : createPath(to); | ||
} | ||
|
||
let history: MemoryHistory = { | ||
get index() { | ||
return index; | ||
|
@@ -265,8 +276,9 @@ export function createMemoryHistory( | |
get location() { | ||
return getCurrentLocation(); | ||
}, | ||
createHref(to) { | ||
return typeof to === "string" ? to : createPath(to); | ||
createHref, | ||
createURL(to) { | ||
return new URL(createHref(to), "http://localhost"); | ||
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. memory histories will use localhost as the URL base |
||
}, | ||
encodeLocation(to: To) { | ||
let path = typeof to === "string" ? parsePath(to) : to; | ||
|
@@ -558,24 +570,6 @@ export function parsePath(path: string): Partial<Path> { | |
return parsedPath; | ||
} | ||
|
||
export function createClientSideURL(location: Location | string): URL { | ||
// window.location.origin is "null" (the literal string value) in Firefox | ||
// under certain conditions, notably when serving from a local HTML file | ||
// See https://bugzilla.mozilla.org/show_bug.cgi?id=878297 | ||
let base = | ||
typeof window !== "undefined" && | ||
typeof window.location !== "undefined" && | ||
window.location.origin !== "null" | ||
? window.location.origin | ||
: window.location.href; | ||
let href = typeof location === "string" ? location : createPath(location); | ||
invariant( | ||
base, | ||
`No window.location.(origin|href) available to create URL for href: ${href}` | ||
); | ||
return new URL(href, base); | ||
} | ||
|
||
export interface UrlHistory extends History {} | ||
|
||
export type UrlHistoryOptions = { | ||
|
@@ -637,6 +631,23 @@ function getUrlBasedHistory( | |
} | ||
} | ||
|
||
function createURL(to: To): URL { | ||
// window.location.origin is "null" (the literal string value) in Firefox | ||
// under certain conditions, notably when serving from a local HTML file | ||
// See https://bugzilla.mozilla.org/show_bug.cgi?id=878297 | ||
let base = | ||
window.location.origin !== "null" | ||
? window.location.origin | ||
: window.location.href; | ||
|
||
let href = createHref(window, to); | ||
invariant( | ||
base, | ||
`No window.location.(origin|href) available to create URL for href: ${href}` | ||
); | ||
return new URL(href, base); | ||
} | ||
|
||
let history: History = { | ||
get action() { | ||
return action; | ||
|
@@ -659,11 +670,10 @@ function getUrlBasedHistory( | |
createHref(to) { | ||
return createHref(window, to); | ||
}, | ||
createURL, | ||
encodeLocation(to) { | ||
// Encode a Location the same way window.location would | ||
let url = createClientSideURL( | ||
typeof to === "string" ? to : createPath(to) | ||
); | ||
let url = createURL(to); | ||
return { | ||
pathname: url.pathname, | ||
search: url.search, | ||
|
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.
Unsure why we had 2 ways of doing this but I moved the custom environment stuff into
setup.ts