Skip to content

Commit

Permalink
Slight adjustment to approach
Browse files Browse the repository at this point in the history
  • Loading branch information
brophdawg11 committed Jun 13, 2023
1 parent 8738524 commit 8c18d95
Show file tree
Hide file tree
Showing 5 changed files with 57 additions and 61 deletions.
5 changes: 5 additions & 0 deletions .changeset/dual-forward-bug.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/react": patch
---

Detect mismatches between the initially loaded URL and the URL at the time we hydrate and trigger a hard reload if they do not match. This is an edge-case that can happen when the network is slowish and the user clicks forward into a Remix app and then clicks forward again while the initial JS chunks are loading.
83 changes: 33 additions & 50 deletions integration/browser-entry-test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import type { Request } from "@playwright/test";
import { test, expect } from "@playwright/test";

import type { AppFixture, Fixture } from "./helpers/create-fixture";
Expand All @@ -25,11 +24,8 @@ test.beforeAll(async () => {
`,

"app/routes/burgers.jsx": js`
export default function Index() {
return (
<div id="cheeseburger">cheeseburger</div>
);
return <div id="cheeseburger">cheeseburger</div>;
}
`,
},
Expand All @@ -41,61 +37,48 @@ test.beforeAll(async () => {

test.afterAll(() => appFixture.close());

// This test generally fails without the corresponding fix in browser.tsx,
// but sometimes manages to pass. With the fix, it always passes.
test(`expect to be able to browse backward out of a remix app,
then forward in history and have pages render correctly`, async ({
page,
browserName,
}) => {
test.skip(
browserName === "firefox",
"FireFox doesn't support browsing to an empty page (aka about:blank)"
);

let app = new PlaywrightFixture(appFixture, page);

// This sets up the Remix modules cache in memory, priming the error case.
await app.goto("/");
await app.clickLink("/burgers");
expect(await page.content()).toContain("cheeseburger");
test(
"expect to be able to browse backward out of a remix app, then forward " +
"twice in history and have pages render correctly",
async ({ page, browserName }) => {
test.skip(
browserName === "firefox",
"FireFox doesn't support browsing to an empty page (aka about:blank)"
);

let app = new PlaywrightFixture(appFixture, page);

// Slow down the entry chunk on the second load so the bug surfaces
let isSecondLoad = false;
await page.route(/entry/, async (route) => {
if (isSecondLoad) {
await new Promise((r) => setTimeout(r, 1000));
}
route.continue();
});

let retry = 4;
for (let i = 0; i < retry; i++) {
// Back to /
// This sets up the Remix modules cache in memory, priming the error case.
await app.goto("/");
await app.clickLink("/burgers");
expect(await page.content()).toContain("cheeseburger");
await page.goBack();

await page.waitForSelector("#pizza");
expect(await app.getHtml()).toContain("pizza");

// Takes the browser to an empty state.
// This doesn't seem to work in headless Firefox.
// Takes the browser out of the Remix app
await page.goBack();
expect(page.url()).toContain("about:blank");

// This attempts to watch for the request for the entry.client.js chunk
// and redirect before it is finished loading.
let redirectOnEntryChunk = async (request: Request) => {
if (request.url().includes("entry")) {
page.off("request", redirectOnEntryChunk);
await page.goForward();
}
};

page.on("request", redirectOnEntryChunk);

// Forward to /
// This initiates a request for the entry.client.js chunk
// Forward to / and immediately again to /burgers. This will trigger the
// error since we'll load __routeModules for / but then try to hydrate /burgers
isSecondLoad = true;
await page.goForward();
await page.goForward();
expect(page.url()).toContain("/");

// The navigation to /burgers happens in `redirectOnEntryChunk`.
// Here's an error: the path should be `/burgers`
// (this validates correctly and passes)
await page.waitForSelector("#cheeseburger");
expect(page.url()).toContain("/burgers");

// but now the content won't contain the string "cheeseburger"
// If we resolve the error, we should hard reload and eventually
// successfully render /burgers
await page.waitForSelector("#cheeseburger");
expect(await app.getHtml()).toContain("cheeseburger");
}
});
);
27 changes: 16 additions & 11 deletions packages/remix-react/browser.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
/* eslint-disable prefer-let/prefer-let */
declare global {
var __remixContext: {
url: string;
state: HydrationState;
future: FutureConfig;
// The number of active deferred keys rendered on the server
Expand Down Expand Up @@ -177,6 +178,21 @@ export function RemixBrowser(_props: RemixBrowserProps): ReactElement {
window.__remixContext.future.v2_normalizeFormMethod,
},
});

// Hard reload if the URL we tried to load is not the current URL.
// This is usually the result of 2 rapid backwards/forward clicks from an
// external site into a Remix app, where we initially start the load for
// one URL and while the JS chunks are loading a second forward click moves
// us to a new URL
let initialUrl = window.__remixContext.url;
let hydratedUrl = window.location.pathname + window.location.search;
if (initialUrl !== hydratedUrl) {
let errorMsg =
`Initial URL (${initialUrl}) does not match URL at time of hydration ` +
`(${hydratedUrl}), reloading page...`;
console.error(errorMsg);
window.location.reload();
}
}

let [location, setLocation] = React.useState(router.state.location);
Expand All @@ -189,17 +205,6 @@ export function RemixBrowser(_props: RemixBrowserProps): ReactElement {
});
}, [location]);

// Check if __remixRouteModules is missing the module for the current route
// and if so, load it.
for (let match of router.state.matches) {
if (!window.__remixRouteModules[match.route.id]) {
window.location.reload();
// `reload` executes asynchronously, meaning the error will still flash.
// To get around that we return an empty div.
return <div/>;
}
}

// We need to include a wrapper RemixErrorBoundary here in case the root error
// boundary also throws and we need to bubble up outside of the router entirely.
// Then we need a stateful location here so the user can back-button navigate
Expand Down
2 changes: 2 additions & 0 deletions packages/remix-server-runtime/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,7 @@ async function handleDocumentRequestRR(
routeModules: createEntryRouteModules(build.routes),
staticHandlerContext: context,
serverHandoffString: createServerHandoffString({
url: context.location.pathname + context.location.search,
state: {
loaderData: context.loaderData,
actionData: context.actionData,
Expand Down Expand Up @@ -307,6 +308,7 @@ async function handleDocumentRequestRR(
...entryContext,
staticHandlerContext: context,
serverHandoffString: createServerHandoffString({
url: context.location.pathname + context.location.search,
state: {
loaderData: context.loaderData,
actionData: context.actionData,
Expand Down
1 change: 1 addition & 0 deletions packages/remix-server-runtime/serverHandoff.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export function createServerHandoffString<T>(serverHandoff: {
// Don't allow StaticHandlerContext to be passed in verbatim, since then
// we'd end up including duplicate info
state: ValidateShape<T, HydrationState>;
url: string;
future: FutureConfig;
dev?: { websocketPort: number };
}): string {
Expand Down

0 comments on commit 8c18d95

Please sign in to comment.