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

Disable FOW for all ssr:false and tighten up spa mode logic #12894

Merged
merged 3 commits into from
Jan 30, 2025
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
9 changes: 9 additions & 0 deletions .changeset/unlucky-dolphins-chew.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
"@react-router/dev": patch
"react-router": patch
---

Disable Lazy Route Discovery for all `ssr:false` app and not just "SPA Mode" because there is no runtime server to serve the search-param-configured `/__manifest` requests
brophdawg11 marked this conversation as resolved.
Show resolved Hide resolved

- We previously only disabled this for "SPA Mode" which is `ssr:false` and no `prerender` config but we realized it should apply to all `ssr:false` apps, including those prerendeirng multiple pages.
brophdawg11 marked this conversation as resolved.
Show resolved Hide resolved
- In those `prerender` scenarios we would prerender the `/__manifest` file assuming the static file server would serve it but that makes some unneccesary assumptions about the static file server behaviors
21 changes: 13 additions & 8 deletions integration/vite-prerender-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,6 @@ test.describe("Prerendering", () => {

let clientDir = path.join(fixture.projectDir, "build", "client");
expect(listAllFiles(clientDir).sort()).toEqual([
"__manifest",
"_root.data",
"about.data",
"about/index.html",
Expand Down Expand Up @@ -288,7 +287,6 @@ test.describe("Prerendering", () => {

let clientDir = path.join(fixture.projectDir, "build", "client");
expect(listAllFiles(clientDir).sort()).toEqual([
"__manifest",
"_root.data",
"about.data",
"about/index.html",
Expand Down Expand Up @@ -345,7 +343,6 @@ test.describe("Prerendering", () => {

let clientDir = path.join(fixture.projectDir, "build", "client");
expect(listAllFiles(clientDir).sort()).toEqual([
"__manifest",
"_root.data",
"a.data",
"a/index.html",
Expand Down Expand Up @@ -397,7 +394,6 @@ test.describe("Prerendering", () => {

let clientDir = path.join(fixture.projectDir, "build", "client");
expect(listAllFiles(clientDir).sort()).toEqual([
"__manifest",
"_root.data",
"about.data",
"about/index.html",
Expand Down Expand Up @@ -468,7 +464,6 @@ test.describe("Prerendering", () => {

let clientDir = path.join(fixture.projectDir, "build", "client");
expect(listAllFiles(clientDir).sort()).toEqual([
"__manifest",
"_root.data",
"about.data",
"about/index.html",
Expand All @@ -494,7 +489,13 @@ test.describe("Prerendering", () => {
test("Hydrates into a navigable app", async ({ page }) => {
fixture = await createFixture({
prerender: true,
files,
files: {
...files,
"react-router.config.ts": reactRouterConfig({
ssr: false,
prerender: true,
}),
},
});
appFixture = await createAppFixture(fixture);

Expand All @@ -511,14 +512,14 @@ test.describe("Prerendering", () => {
await page.waitForSelector("[data-mounted]");
await app.clickLink("/about");
await page.waitForSelector("[data-route]:has-text('About')");
expect(requests).toEqual(["/__manifest", "/about.data"]);
expect(requests).toEqual(["/about.data"]);
});

test("Serves the prerendered HTML file alongside runtime routes", async ({
page,
}) => {
fixture = await createFixture({
// Even thogh we are prerendering, we want a running server so we can
// Even though we are prerendering, we want a running server so we can
// hit the pre-rendered HTML file and a non-prerendered route
prerender: false,
files: {
Expand Down Expand Up @@ -783,6 +784,10 @@ test.describe("Prerendering", () => {
prerender: true,
files: {
...files,
"react-router.config.ts": reactRouterConfig({
ssr: false,
prerender: true,
}),
"app/routes/$slug.tsx": js`
import * as React from "react";
import { useLoaderData } from "react-router";
Expand Down
45 changes: 16 additions & 29 deletions packages/react-router-dev/vite/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -482,9 +482,8 @@ export const reactRouterVitePlugin: ReactRouterVitePlugin = () => {
)};
export const basename = ${JSON.stringify(ctx.reactRouterConfig.basename)};
export const future = ${JSON.stringify(ctx.reactRouterConfig.future)};
export const isSpaMode = ${
!ctx.reactRouterConfig.ssr && ctx.reactRouterConfig.prerender == null
};
export const ssr = ${ctx.reactRouterConfig.ssr};
export const isSpaMode = ${isSpaModeEnabled(ctx.reactRouterConfig)};
export const publicPath = ${JSON.stringify(ctx.publicPath)};
export const entry = { module: entryServer };
export const routes = {
Expand Down Expand Up @@ -1402,7 +1401,7 @@ export const reactRouterVitePlugin: ReactRouterVitePlugin = () => {
let route = getRoute(ctx.reactRouterConfig, id);
if (!route) return;

if (!options?.ssr && !ctx.reactRouterConfig.ssr) {
if (!options?.ssr && isSpaModeEnabled(ctx.reactRouterConfig)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only reject server-only exports in true SPA mode

let serverOnlyExports = esModuleLexer(code)[1]
.map((exp) => exp.n)
.filter((exp) => SERVER_ONLY_ROUTE_EXPORTS.includes(exp));
Expand Down Expand Up @@ -1774,6 +1773,19 @@ async function getRouteMetadata(
return info;
}

function isSpaModeEnabled(
reactRouterConfig: ReactRouterPluginContext["reactRouterConfig"]
) {
return (
reactRouterConfig.ssr === false &&
(reactRouterConfig.prerender == null ||
reactRouterConfig.prerender === false ||
(Array.isArray(reactRouterConfig.prerender) &&
reactRouterConfig.prerender.length === 1 &&
reactRouterConfig.prerender[0] === "/"))
);
}
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 proper detection, the old buggy one was simply:

!ctx.reactRouterConfig.ssr && ctx.reactRouterConfig.prerender == null


async function getPrerenderBuildAndHandler(
viteConfig: Vite.ResolvedConfig,
serverBuildDirectory: string,
Expand Down Expand Up @@ -1908,13 +1920,6 @@ async function handlePrerender(
);
}
}

await prerenderManifest(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Never need to prerender a manifest anymore

  • ssr: true will handle the manifest via the server handler
  • ssr: false will not have fog of war enabled

build,
clientBuildDirectory,
reactRouterConfig,
viteConfig
);
}

function determineStaticPrerenderRoutes(
Expand Down Expand Up @@ -2042,24 +2047,6 @@ async function prerenderResourceRoute(
viteConfig.logger.info(`Prerender: Generated ${colors.bold(outfile)}`);
}

async function prerenderManifest(
build: ServerBuild,
clientBuildDirectory: string,
reactRouterConfig: ResolvedReactRouterConfig,
viteConfig: Vite.ResolvedConfig
) {
let normalizedPath = `${reactRouterConfig.basename}/__manifest`.replace(
/\/\/+/g,
"/"
);
let outdir = path.relative(process.cwd(), clientBuildDirectory);
let outfile = path.join(outdir, ...normalizedPath.split("/"));
await fse.ensureDir(path.dirname(outfile));
let manifestData = JSON.stringify(build.assets.routes);
await fse.outputFile(outfile, manifestData);
viteConfig.logger.info(`Prerender: Generated ${colors.bold(outfile)}`);
}

function validatePrerenderedResponse(
response: Response,
html: string,
Expand Down
3 changes: 3 additions & 0 deletions packages/react-router/lib/dom-export/hydrated-router.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ function createHydratedRouter(): DataRouter {
patchRoutesOnNavigation: getPatchRoutesOnNavigationFunction(
ssrInfo.manifest,
ssrInfo.routeModules,
ssrInfo.context.ssr,
ssrInfo.context.isSpaMode,
ssrInfo.context.basename
),
Expand Down Expand Up @@ -247,6 +248,7 @@ export function HydratedRouter() {
router,
ssrInfo.manifest,
ssrInfo.routeModules,
ssrInfo.context.ssr,
ssrInfo.context.isSpaMode
);

Expand All @@ -264,6 +266,7 @@ export function HydratedRouter() {
routeModules: ssrInfo.routeModules,
future: ssrInfo.context.future,
criticalCss,
ssr: ssrInfo.context.ssr,
isSpaMode: ssrInfo.context.isSpaMode,
}}
>
Expand Down
1 change: 1 addition & 0 deletions packages/react-router/lib/dom/global.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ export type WindowReactRouterContext = {
state: HydrationState;
criticalCss?: string;
future: FutureConfig;
ssr: boolean;
isSpaMode: boolean;
stream: ReadableStream<Uint8Array> | undefined;
streamController: ReadableStreamDefaultController<Uint8Array>;
Expand Down
4 changes: 2 additions & 2 deletions packages/react-router/lib/dom/ssr/components.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -638,11 +638,11 @@ export type ScriptsProps = Omit<
@category Components
*/
export function Scripts(props: ScriptsProps) {
let { manifest, serverHandoffString, isSpaMode, renderMeta } =
let { manifest, serverHandoffString, isSpaMode, ssr, renderMeta } =
useFrameworkContext();
let { router, static: isStatic, staticContext } = useDataRouterContext();
let { matches: routerMatches } = useDataRouterStateContext();
let enableFogOfWar = isFogOfWarEnabled(isSpaMode);
let enableFogOfWar = isFogOfWarEnabled(ssr);

// Let <ServerRouter> know that we hydrated and we should render the single
// fetch streaming scripts
Expand Down
1 change: 1 addition & 0 deletions packages/react-router/lib/dom/ssr/entry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export interface FrameworkContextObject {
criticalCss?: string;
serverHandoffString?: string;
future: FutureConfig;
ssr: boolean;
isSpaMode: boolean;
serializeError?(error: Error): SerializedError;
renderMeta?: {
Expand Down
15 changes: 7 additions & 8 deletions packages/react-router/lib/dom/ssr/fog-of-war.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ const discoveredPaths = new Set<string>();
// https://stackoverflow.com/a/417184
const URL_LIMIT = 7680;

export function isFogOfWarEnabled(isSpaMode: boolean) {
return !isSpaMode;
export function isFogOfWarEnabled(ssr: boolean) {
return ssr === true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only enable fog of war for ssr:true apps

}

export function getPartialManifest(
Expand Down Expand Up @@ -70,10 +70,11 @@ export function getPartialManifest(
export function getPatchRoutesOnNavigationFunction(
manifest: AssetsManifest,
routeModules: RouteModules,
ssr: boolean,
isSpaMode: boolean,
basename: string | undefined
): PatchRoutesOnNavigationFunction | undefined {
if (!isFogOfWarEnabled(isSpaMode)) {
if (!isFogOfWarEnabled(ssr)) {
return undefined;
}

Expand All @@ -96,14 +97,12 @@ export function useFogOFWarDiscovery(
router: DataRouter,
manifest: AssetsManifest,
routeModules: RouteModules,
ssr: boolean,
isSpaMode: boolean
) {
React.useEffect(() => {
// Don't prefetch if not enabled or if the user has `saveData` enabled
if (
!isFogOfWarEnabled(isSpaMode) ||
navigator.connection?.saveData === true
) {
if (!isFogOfWarEnabled(ssr) || navigator.connection?.saveData === true) {
return;
}

Expand Down Expand Up @@ -176,7 +175,7 @@ export function useFogOFWarDiscovery(
});

return () => observer.disconnect();
}, [isSpaMode, manifest, routeModules, router]);
}, [ssr, isSpaMode, manifest, routeModules, router]);
}

export async function fetchAndApplyManifestPatches(
Expand Down
1 change: 1 addition & 0 deletions packages/react-router/lib/dom/ssr/routes-test-stub.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ export function createRoutesStub(
version: "",
},
routeModules: {},
ssr: false,
isSpaMode: false,
};

Expand Down
1 change: 1 addition & 0 deletions packages/react-router/lib/dom/ssr/server.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ export function ServerRouter({
criticalCss,
serverHandoffString,
future: context.future,
ssr: context.ssr,
isSpaMode: context.isSpaMode,
serializeError: context.serializeError,
renderMeta: context.renderMeta,
Expand Down
1 change: 1 addition & 0 deletions packages/react-router/lib/server-runtime/build.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export interface ServerBuild {
publicPath: string;
assetsBuildDirectory: string;
future: FutureConfig;
ssr: boolean;
isSpaMode: boolean;
}

Expand Down
3 changes: 3 additions & 0 deletions packages/react-router/lib/server-runtime/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,7 @@ async function handleDocumentRequest(
basename: build.basename,
criticalCss,
future: build.future,
ssr: build.ssr,
isSpaMode: build.isSpaMode,
}),
serverHandoffStream: encodeViaTurboStream(
Expand All @@ -401,6 +402,7 @@ async function handleDocumentRequest(
),
renderMeta: {},
future: build.future,
ssr: build.ssr,
isSpaMode: build.isSpaMode,
serializeError: (err) => serializeError(err, serverMode),
};
Expand Down Expand Up @@ -461,6 +463,7 @@ async function handleDocumentRequest(
serverHandoffString: createServerHandoffString({
basename: build.basename,
future: build.future,
ssr: build.ssr,
isSpaMode: build.isSpaMode,
}),
serverHandoffStream: encodeViaTurboStream(
Expand Down
1 change: 1 addition & 0 deletions packages/react-router/lib/server-runtime/serverHandoff.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export function createServerHandoffString<T>(serverHandoff: {
criticalCss?: string;
basename: string | undefined;
future: FutureConfig;
ssr: boolean;
isSpaMode: boolean;
}): string {
// Uses faster alternative of jsesc to escape data returned from the loaders.
Expand Down