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.

Sign up for GitHub

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 serverBundles issue with multiple browser manifests #8864

Merged
merged 2 commits into from
Feb 23, 2024
Merged

Fix serverBundles issue with multiple browser manifests #8864

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d2d7f41
Fix `serverBundles` issue with multiple browser manifests
markdalgleish Feb 23, 2024
6c03682
Fix type
markdalgleish Feb 23, 2024
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/silver-years-roll.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/dev": patch
---

Vite: Fix `serverBundles` issue where multiple browser manifests are generated
11 changes: 11 additions & 0 deletions integration/vite-server-bundles-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,17 @@ test.describe(() => {
expect(pageErrors).toEqual([]);
});

test("Vite / server bundles / build / Remix browser manifest", () => {
let clientAssetFiles = fs.readdirSync(
path.join(cwd, "build", "client", "assets")
);
let manifestFiles = clientAssetFiles.filter((filename) =>
filename.startsWith("manifest-")
);

expect(manifestFiles.length).toEqual(1);
});

test("Vite / server bundles / build / Vite manifests", () => {
let viteManifestFiles = fs.readdirSync(path.join(cwd, "build", ".vite"));

Expand Down
106 changes: 65 additions & 41 deletions packages/remix-dev/vite/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import {
type RemixConfig as ResolvedRemixEsbuildConfig,
resolveConfig as resolveRemixEsbuildConfig,
} from "../config";
import { type Manifest as BrowserManifest } from "../manifest";
import { type Manifest as RemixManifest } from "../manifest";
Copy link
Member Author

Choose a reason for hiding this comment

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

This type has been renamed for clarity since the manifest type is shared between client and server, and especially now that its contents can differ too.

import invariant from "../invariant";
import {
type NodeRequestHandler,
Expand Down Expand Up @@ -200,13 +200,13 @@ export type ServerBundleBuildConfig = {
type RemixPluginSsrBuildContext =
| {
isSsrBuild: false;
getBrowserManifest?: never;
serverBundleId?: never;
getRemixServerManifest?: never;
serverBundleBuildConfig?: never;
}
| {
isSsrBuild: true;
getBrowserManifest: () => Promise<BrowserManifest>;
serverBundleId: string | undefined;
getRemixServerManifest: () => Promise<RemixManifest>;
serverBundleBuildConfig: ServerBundleBuildConfig | null;
};

export type RemixPluginContext = RemixPluginSsrBuildContext & {
Expand Down Expand Up @@ -286,7 +286,7 @@ const resolveBuildAssetPaths = (
viteManifest: Vite.Manifest,
entryFilePath: string,
prependedAssetFilePaths: string[] = []
): BrowserManifest["entry"] & { css: string[] } => {
): RemixManifest["entry"] & { css: string[] } => {
let entryChunk = resolveChunk(ctx, viteManifest, entryFilePath);

// This is here to support prepending client entry assets to the root route
Expand Down Expand Up @@ -423,7 +423,9 @@ export let getServerBuildDirectory = (ctx: RemixPluginContext) =>
path.join(
ctx.remixConfig.buildDirectory,
"server",
...(typeof ctx.serverBundleId === "string" ? [ctx.serverBundleId] : [])
...(ctx.serverBundleBuildConfig
? [ctx.serverBundleBuildConfig.serverBundleId]
: [])
);

let getClientBuildDirectory = (remixConfig: ResolvedVitePluginConfig) =>
Expand Down Expand Up @@ -638,15 +640,6 @@ export const remixVitePlugin: RemixVitePlugin = (remixUserConfig = {}) => {
serverBundles = undefined;
}

// Get the server bundle build config injected by the Remix CLI, if present.
let serverBundleBuildConfig = getServerBundleBuildConfig(viteUserConfig);

// For server bundle builds, override the relevant config. This lets us run
// multiple server builds with each one targeting a subset of routes.
if (serverBundleBuildConfig) {
routes = serverBundleBuildConfig.routes;
Copy link
Member Author

Choose a reason for hiding this comment

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

This was the crux of the issue. Both client and server received a subset of routes when in reality only the server should have a subset.

}

let remixConfig: ResolvedVitePluginConfig = deepFreeze({
appDirectory,
basename,
Expand All @@ -672,9 +665,9 @@ export const remixVitePlugin: RemixVitePlugin = (remixUserConfig = {}) => {
viteConfigEnv.isSsrBuild && viteCommand === "build"
? {
isSsrBuild: true,
getBrowserManifest: createBrowserManifestForBuild,
serverBundleId:
getServerBundleBuildConfig(viteUserConfig)?.serverBundleId,
getRemixServerManifest: async () =>
(await generateRemixManifestsForBuild()).remixServerManifest,
serverBundleBuildConfig: getServerBundleBuildConfig(viteUserConfig),
}
: { isSsrBuild: false };

Expand All @@ -701,13 +694,20 @@ export const remixVitePlugin: RemixVitePlugin = (remixUserConfig = {}) => {
// mark the old compiler as deprecated
// - Remove `ServerBuild.mode` in v3

let routes = ctx.serverBundleBuildConfig
? // For server bundle builds, the server build should only import the
// routes for this bundle rather than importing all routes
ctx.serverBundleBuildConfig.routes
: // Otherwise, all routes are imported as usual
ctx.remixConfig.routes;
Comment on lines +697 to +702
Copy link
Member Author

Choose a reason for hiding this comment

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

This is one of two main parts of the fix. Rather than modifying the resolved routes for the Remix config — which incorrectly impacts the client, not just the server — we use a subset of routes in the server build file.


return `
import * as entryServer from ${JSON.stringify(
resolveFileUrl(ctx, ctx.entryServerFilePath)
)};
${Object.keys(ctx.remixConfig.routes)
${Object.keys(routes)
.map((key, index) => {
let route = ctx.remixConfig.routes[key]!;
let route = routes[key]!;
return `import * as route${index} from ${JSON.stringify(
resolveFileUrl(
ctx,
Expand All @@ -734,9 +734,9 @@ export const remixVitePlugin: RemixVitePlugin = (remixUserConfig = {}) => {
export const publicPath = ${JSON.stringify(ctx.remixConfig.publicPath)};
export const entry = { module: entryServer };
export const routes = {
${Object.keys(ctx.remixConfig.routes)
${Object.keys(routes)
.map((key, index) => {
let route = ctx.remixConfig.routes[key]!;
let route = routes[key]!;
return `${JSON.stringify(key)}: {
id: ${JSON.stringify(route.id)},
parentId: ${JSON.stringify(route.parentId)},
Expand Down Expand Up @@ -774,7 +774,10 @@ export const remixVitePlugin: RemixVitePlugin = (remixUserConfig = {}) => {
return new Set([...cssUrlPaths, ...chunkAssetPaths]);
};

let createBrowserManifestForBuild = async (): Promise<BrowserManifest> => {
let generateRemixManifestsForBuild = async (): Promise<{
remixBrowserManifest: RemixManifest;
remixServerManifest: RemixManifest;
}> => {
let viteManifest = await loadViteManifest(
getClientBuildDirectory(ctx.remixConfig)
);
Expand All @@ -785,7 +788,8 @@ export const remixVitePlugin: RemixVitePlugin = (remixUserConfig = {}) => {
ctx.entryClientFilePath
);

let routes: BrowserManifest["routes"] = {};
let browserRoutes: RemixManifest["routes"] = {};
let serverRoutes: RemixManifest["routes"] = {};

let routeManifestExports = await getRouteManifestModuleExports(
viteChildCompiler,
Expand All @@ -797,7 +801,7 @@ export const remixVitePlugin: RemixVitePlugin = (remixUserConfig = {}) => {
let sourceExports = routeManifestExports[key];
let isRootRoute = route.parentId === undefined;

routes[key] = {
let routeManifestEntry = {
id: route.id,
parentId: route.parentId,
path: route.path,
Expand All @@ -818,29 +822,49 @@ export const remixVitePlugin: RemixVitePlugin = (remixUserConfig = {}) => {
isRootRoute ? [ctx.entryClientFilePath] : []
),
};

browserRoutes[key] = routeManifestEntry;

let serverBundleRoutes = ctx.serverBundleBuildConfig?.routes;
if (!serverBundleRoutes || serverBundleRoutes[key]) {
serverRoutes[key] = routeManifestEntry;
}
}

let fingerprintedValues = { entry, routes };
let fingerprintedValues = { entry, routes: browserRoutes };
let version = getHash(JSON.stringify(fingerprintedValues), 8);
let manifestPath = `assets/manifest-${version}.js`;
let url = `${ctx.remixConfig.publicPath}${manifestPath}`;
let nonFingerprintedValues = { url, version };

let manifest: BrowserManifest = {
let remixBrowserManifest: RemixManifest = {
...fingerprintedValues,
...nonFingerprintedValues,
};

// Write the browser manifest to disk as part of the build process
await writeFileSafe(
path.join(getClientBuildDirectory(ctx.remixConfig), manifestPath),
`window.__remixManifest=${JSON.stringify(manifest)};`
`window.__remixManifest=${JSON.stringify(remixBrowserManifest)};`
);

return manifest;
// The server manifest is the same as the browser manifest, except for
// server bundle builds which only includes routes for the current bundle,
// otherwise the server and client have the same routes
let remixServerManifest: RemixManifest = {
...remixBrowserManifest,
routes: serverRoutes,
};
Comment on lines +851 to +857
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the other main part of the fix. Here we only provide a subset of the manifest on the server when using server bundles, while still writing the full manifest to disk for the client.


return {
remixBrowserManifest,
remixServerManifest,
};
};

let getBrowserManifestForDev = async (): Promise<BrowserManifest> => {
let routes: BrowserManifest["routes"] = {};
// In dev, the server and browser Remix manifests are the same
let getRemixManifestForDev = async (): Promise<RemixManifest> => {
let routes: RemixManifest["routes"] = {};

let routeManifestExports = await getRouteManifestModuleExports(
viteChildCompiler,
Expand Down Expand Up @@ -1328,21 +1352,21 @@ export const remixVitePlugin: RemixVitePlugin = (remixUserConfig = {}) => {
return await getServerEntry();
}
case VirtualModule.resolve(serverManifestId): {
let browserManifest = ctx.isSsrBuild
? await ctx.getBrowserManifest()
: await getBrowserManifestForDev();
let remixManifest = ctx.isSsrBuild
? await ctx.getRemixServerManifest()
: await getRemixManifestForDev();

return `export default ${jsesc(browserManifest, { es6: true })};`;
return `export default ${jsesc(remixManifest, { es6: true })};`;
}
case VirtualModule.resolve(browserManifestId): {
if (viteCommand === "build") {
throw new Error("This module only exists in development");
}

let browserManifest = await getBrowserManifestForDev();
let browserManifestString = jsesc(browserManifest, { es6: true });
let remixManifest = await getRemixManifestForDev();
let remixManifestString = jsesc(remixManifest, { es6: true });

return `window.__remixManifest=${browserManifestString};`;
return `window.__remixManifest=${remixManifestString};`;
}
}
},
Expand Down Expand Up @@ -1567,14 +1591,14 @@ export const remixVitePlugin: RemixVitePlugin = (remixUserConfig = {}) => {
async handleHotUpdate({ server, file, modules, read }) {
let route = getRoute(ctx.remixConfig, file);

type ManifestRoute = BrowserManifest["routes"][string];
type ManifestRoute = RemixManifest["routes"][string];
type HmrEventData = { route: ManifestRoute | null };
let hmrEventData: HmrEventData = { route: null };

if (route) {
// invalidate manifest on route exports change
let serverManifest = (await server.ssrLoadModule(serverManifestId))
.default as BrowserManifest;
.default as RemixManifest;

let oldRouteMetadata = serverManifest.routes[route.id];
let newRouteMetadata = await getRouteMetadata(
Expand Down