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 HDR for single fetch #9954

Merged
merged 2 commits into from
Sep 6, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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/rude-cows-brake.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/react": patch
---

[REMOVE] Fix HDR for single fetch
56 changes: 56 additions & 0 deletions integration/vite-hmr-hdr-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
EXPRESS_SERVER,
viteConfig,
} from "./helpers/vite.js";
import { js } from "./helpers/create-fixture.js";

const indexRoute = `
// imports
Expand Down Expand Up @@ -112,6 +113,61 @@ test("Vite / HMR & HDR / mdx", async ({ page, viteDev }) => {
expect(page.errors).toEqual([]);
});

test.describe("single fetch", () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't have copies of these tests with the flag enabled so didn't catch this until we migrated the changes to RR v7 where single fetch is always on

test("Vite / HMR & HDR / vite dev", async ({
page,
browserName,
viteDev,
}) => {
let files: Files = async ({ port }) => ({
"vite.config.js": js`
import { vitePlugin as remix } from "@remix-run/dev";

export default {
${await viteConfig.server({ port })}
plugins: [
remix({
future: {
unstable_singleFetch: true
},
})
]
}
`,
"app/routes/_index.tsx": indexRoute,
});
let { cwd, port } = await viteDev(files);
await workflow({ page, browserName, cwd, port });
});

test("Vite / HMR & HDR / express", async ({
page,
browserName,
customDev,
}) => {
let files: Files = async ({ port }) => ({
"vite.config.js": js`
import { vitePlugin as remix } from "@remix-run/dev";

export default {
${await viteConfig.server({ port })}
plugins: [
remix({
future: {
unstable_singleFetch: true
},
})
]
}
`,
"server.mjs": EXPRESS_SERVER({ port }),
"app/routes/_index.tsx": indexRoute,
});
let { cwd, port } = await customDev(files);
await workflow({ page, browserName, cwd, port });
});
});

async function workflow({
page,
browserName,
Expand Down
8 changes: 7 additions & 1 deletion packages/remix-dev/vite/static/refresh-utils.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,13 @@ const enqueueUpdate = debounce(async () => {
window.__remixRouteModuleUpdates.clear();
}

await revalidate();
try {
window.__remixHdrActive = true;
await revalidate();
} finally {
window.__remixHdrActive = false;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to know during the single fetch dataStrategy if this is an HDR revalidation so we can force a server call since the route may have a new loader on the server we don't yet know about


if (manifest) {
Object.assign(window.__remixManifest, manifest);
}
Expand Down
1 change: 1 addition & 0 deletions packages/remix-react/browser.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ declare global {
var __remixRouteModules: RouteModules;
var __remixManifest: AssetsManifest;
var __remixRevalidation: number | undefined;
var __remixHdrActive: boolean;
var __remixClearCriticalCss: (() => void) | undefined;
var $RefreshRuntime$: {
performReactRefresh: () => void;
Expand Down
56 changes: 29 additions & 27 deletions packages/remix-react/single-fetch.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -261,46 +261,48 @@ async function singleFetchLoaderNavigationStrategy(
results[m.route.id] = { type: "error", result: e };
}
return;
} else if (!manifest.routes[m.route.id].hasLoader) {
// If we don't have a server loader, then we don't care about the HTTP
// call and can just send back a `null` - because we _do_ have a `loader`
// in the client router handling route module/styles loads
results[m.route.id] = {
type: "data",
result: null,
};
Comment on lines -268 to -271
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 was unnecessary - should instead be assigning to results based on the result of handler() below - not inside in the handlerOverride. This will properly handle these types of routes which never make it to handlerOverride

return;
}

// Otherwise, we want to load this route on the server and can lump this
// it in with the others on a singular promise
routesParams.add(m.route.id);
// Load this route on the server if it has a loader
if (manifest.routes[m.route.id].hasLoader) {
routesParams.add(m.route.id);
}

await handler(async () => {
try {
// Lump this match in with the others on a singular promise
try {
let result = await handler(async () => {
let data = await singleFetchDfd.promise;
results[m.route.id] = {
type: "data",
result: unwrapSingleFetchResults(data, m.route.id),
};
} catch (e) {
results[m.route.id] = {
type: "error",
result: e,
};
}
});
return unwrapSingleFetchResults(data, m.route.id);
});
results[m.route.id] = {
type: "data",
result,
};
return result;
} catch (e) {
results[m.route.id] = {
type: "error",
result: e,
};
}
})
)
);

// Wait for all routes to resolve above before we make the HTTP call
await routesLoadedPromise;

// Don't make any single fetch server calls:
// We can skip the server call:
// - On initial hydration - only clientLoaders can pass through via `clientLoader.hydrate`
// - If there are no routes to fetch from the server
if (!router.state.initialized || routesParams.size === 0) {
//
// One exception - if we are performing an HDR revalidation we have to call
// the server in case a new loader has shown up that the manifest doesn't yet
// know about
if (
(!router.state.initialized || routesParams.size === 0) &&
!window.__remixHdrActive
) {
singleFetchDfd.resolve({});
} else {
try {
Expand Down
Loading