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

fix(router): fix URL creation in Cloudflare Pages #9682

Merged
merged 5 commits into from
Dec 5, 2022
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/new-kiwis-confess.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/router": patch
---

Fix URL creation in Cloudflare Pages or other non-browser-environment
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@
},
"filesize": {
"packages/router/dist/router.umd.min.js": {
"none": "35 kB"
"none": "35.5 kB"
},
"packages/react-router/dist/react-router.production.min.js": {
"none": "12.5 kB"
Expand Down
26 changes: 23 additions & 3 deletions packages/router/history.ts
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,20 @@ export function createHashHistory(
//#region UTILS
////////////////////////////////////////////////////////////////////////////////

/**
* @private
*/
export function invariant(value: boolean, message?: string): asserts value;
export function invariant<T>(
value: T | null | undefined,
message?: string
): asserts value is T;
export function invariant(value: any, message?: string) {
if (value === false || value === null || typeof value === "undefined") {
throw new Error(message);
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved from utils.ts to here to avoid a circular dependency


function warning(cond: any, message: string) {
if (!cond) {
// eslint-disable-next-line no-console
Expand Down Expand Up @@ -544,7 +558,7 @@ export function parsePath(path: string): Partial<Path> {
return parsedPath;
}

export function createURL(location: Location | string): URL {
export function createClientSideURL(location: Location | string): URL {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make this more apparent that it's not intended to be used in staticHandler

// 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
Expand All @@ -553,8 +567,12 @@ export function createURL(location: Location | string): URL {
typeof window.location !== "undefined" &&
window.location.origin !== "null"
? window.location.origin
: "unknown://unknown";
: 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);
Comment on lines -556 to 576
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The unknown://unknown was a bit of a snap decision very early on, but I think was really a red herring in some unit testing setup issues since new URL('/', 'unknown://unknown') doesn't actually work quite right anyway! Now that we've better defined these APIs as intended only for client-side use - we can just use invariant if we can't detect a proper base.

For the specific Firefox file:// case, unknown://unknown wasn't actually working right and gave incorrect paths - but it didn't throw an error. Instead, firefox does have a "valid" window.location.href we can use as a fallback base when origin = "null" (in file:// mode):

Screenshot 2022-12-05 at 4 21 11 PM

}

Expand Down Expand Up @@ -643,7 +661,9 @@ function getUrlBasedHistory(
},
encodeLocation(to) {
// Encode a Location the same way window.location would
let url = createURL(typeof to === "string" ? to : createPath(to));
let url = createClientSideURL(
typeof to === "string" ? to : createPath(to)
);
return {
pathname: url.pathname,
search: url.search,
Expand Down
3 changes: 1 addition & 2 deletions packages/router/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ export {
defer,
generatePath,
getToPathname,
invariant,
isRouteErrorResponse,
joinPaths,
json,
Expand All @@ -59,13 +58,13 @@ export type {
Path,
To,
} from "./history";

export {
Action,
createBrowserHistory,
createPath,
createHashHistory,
createMemoryHistory,
invariant,
parsePath,
} from "./history";

Expand Down
49 changes: 31 additions & 18 deletions packages/router/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@ import {
Action as HistoryAction,
createLocation,
createPath,
createURL,
createClientSideURL,
invariant,
parsePath,
} from "./history";
import type {
Expand All @@ -28,7 +29,6 @@ import {
ResultType,
convertRoutesToDataRoutes,
getPathContributingMatches,
invariant,
isRouteErrorResponse,
joinPaths,
matchRoutes,
Expand Down Expand Up @@ -913,7 +913,7 @@ export function createRouter(init: RouterInit): Router {

// Create a controller/Request for this navigation
pendingNavigationController = new AbortController();
let request = createRequest(
let request = createClientSideRequest(
location,
pendingNavigationController.signal,
opts && opts.submission
Expand Down Expand Up @@ -954,7 +954,7 @@ export function createRouter(init: RouterInit): Router {
loadingNavigation = navigation;

// Create a GET request for the loaders
request = createRequest(request.url, request.signal);
request = new Request(request.url, { signal: request.signal });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we already have a valid request, we can just inline the request creation. createClientSideRequest is now for when we don't have a request and we just have a path

}

// Call loaders
Expand Down Expand Up @@ -1299,7 +1299,11 @@ export function createRouter(init: RouterInit): Router {

// Call the action for the fetcher
let abortController = new AbortController();
let fetchRequest = createRequest(path, abortController.signal, submission);
let fetchRequest = createClientSideRequest(
path,
abortController.signal,
submission
);
fetchControllers.set(key, abortController);

let actionResult = await callLoaderOrAction(
Expand Down Expand Up @@ -1346,7 +1350,7 @@ export function createRouter(init: RouterInit): Router {
// Start the data load for current matches, or the next location if we're
// in the middle of a navigation
let nextLocation = state.navigation.location || state.location;
let revalidationRequest = createRequest(
let revalidationRequest = createClientSideRequest(
nextLocation,
abortController.signal
);
Expand Down Expand Up @@ -1501,7 +1505,7 @@ export function createRouter(init: RouterInit): Router {

// Call the loader for this fetcher route match
let abortController = new AbortController();
let fetchRequest = createRequest(path, abortController.signal);
let fetchRequest = createClientSideRequest(path, abortController.signal);
fetchControllers.set(key, abortController);
let result: DataResult = await callLoaderOrAction(
"loader",
Expand Down Expand Up @@ -1675,7 +1679,7 @@ export function createRouter(init: RouterInit): Router {
...fetchersToLoad.map(([, href, match, fetchMatches]) =>
callLoaderOrAction(
"loader",
createRequest(href, request.signal),
createClientSideRequest(href, request.signal),
match,
fetchMatches,
router.basename
Expand Down Expand Up @@ -2120,7 +2124,7 @@ export function unstable_createStaticHandler(
if (!actionMatch.route.action) {
let error = getInternalRouterError(405, {
method: request.method,
pathname: createURL(request.url).pathname,
pathname: new URL(request.url).pathname,
routeId: actionMatch.route.id,
});
if (isRouteRequest) {
Expand Down Expand Up @@ -2206,7 +2210,7 @@ export function unstable_createStaticHandler(
}

// Create a GET request for the loaders
let loaderRequest = createRequest(request.url, request.signal);
let loaderRequest = new Request(request.url, { signal: request.signal });
let context = await loadRouteData(loaderRequest, matches);

return {
Expand Down Expand Up @@ -2240,7 +2244,7 @@ export function unstable_createStaticHandler(
if (isRouteRequest && !routeMatch?.route.loader) {
throw getInternalRouterError(400, {
method: request.method,
pathname: createURL(request.url).pathname,
pathname: new URL(request.url).pathname,
routeId: routeMatch?.route.id,
});
}
Expand Down Expand Up @@ -2531,9 +2535,9 @@ function shouldRevalidateLoader(
isRevalidationRequired: boolean,
actionResult: DataResult | undefined
) {
let currentUrl = createURL(currentLocation);
let currentUrl = createClientSideURL(currentLocation);
let currentParams = currentMatch.params;
let nextUrl = createURL(location);
let nextUrl = createClientSideURL(location);
let nextParams = match.params;

// This is the default implementation as to when we revalidate. If the route
Expand Down Expand Up @@ -2624,16 +2628,22 @@ async function callLoaderOrAction(
);

// Check if this an external redirect that goes to a new origin
let external = createURL(location).origin !== createURL("/").origin;
let currentUrl = new URL(request.url);
let currentOrigin = currentUrl.origin;
let newOrigin = new URL(location, currentOrigin).origin;
let external = newOrigin !== currentOrigin;
Comment on lines -2627 to +2634
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 is the main fix for cloudflare - we inline the URL creation and just use the current request to determine the origin


// Support relative routing in internal redirects
if (!external) {
let activeMatches = matches.slice(0, matches.indexOf(match) + 1);
let routePathnames = getPathContributingMatches(activeMatches).map(
(match) => match.pathnameBase
);
let requestPath = createURL(request.url).pathname;
let resolvedLocation = resolveTo(location, routePathnames, requestPath);
let resolvedLocation = resolveTo(
location,
routePathnames,
currentUrl.pathname
);
invariant(
createPath(resolvedLocation),
`Unable to resolve redirect location: ${location}`
Expand Down Expand Up @@ -2713,12 +2723,15 @@ async function callLoaderOrAction(
return { type: ResultType.data, data: result };
}

function createRequest(
// Utility method for creating the Request instances for loaders/actions during
// client-side navigations and fetches. During SSR we will always have a
// Request instance from the static handler (query/queryRoute)
function createClientSideRequest(
location: string | Location,
signal: AbortSignal,
submission?: Submission
): Request {
let url = createURL(stripHashFromPath(location)).toString();
let url = createClientSideURL(stripHashFromPath(location)).toString();
let init: RequestInit = { signal };

if (submission) {
Expand Down
16 changes: 1 addition & 15 deletions packages/router/utils.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { Location, Path, To } from "./history";
import { parsePath } from "./history";
import { invariant, parsePath } from "./history";

/**
* Map of routeId -> data returned from a loader/action/error
Expand Down Expand Up @@ -771,20 +771,6 @@ export function stripBasename(
return pathname.slice(startIndex) || "/";
}

/**
* @private
*/
export function invariant(value: boolean, message?: string): asserts value;
export function invariant<T>(
value: T | null | undefined,
message?: string
): asserts value is T;
export function invariant(value: any, message?: string) {
if (value === false || value === null || typeof value === "undefined") {
throw new Error(message);
}
}

/**
* @private
*/
Expand Down