From c043a6695852967536d6679a8147ba672de9d412 Mon Sep 17 00:00:00 2001 From: Mark Dalgleish Date: Tue, 28 Nov 2023 12:47:58 +1100 Subject: [PATCH] fix(remix-dev/vite): bundle CSS from client entry (#8143) Co-authored-by: Pedro Cattori --- .changeset/brown-hotels-rush.md | 5 + integration/vite-css-build-test.ts | 40 +++++-- integration/vite-css-dev-test.ts | 64 +++++++++-- .../remix-dev/vite/import-vite-esm-sync.ts | 21 ++++ packages/remix-dev/vite/plugin.ts | 102 ++++++++++-------- packages/remix-dev/vite/resolve-file-url.ts | 22 ++++ packages/remix-dev/vite/styles.ts | 54 +++++++--- 7 files changed, 237 insertions(+), 71 deletions(-) create mode 100644 .changeset/brown-hotels-rush.md create mode 100644 packages/remix-dev/vite/import-vite-esm-sync.ts create mode 100644 packages/remix-dev/vite/resolve-file-url.ts diff --git a/.changeset/brown-hotels-rush.md b/.changeset/brown-hotels-rush.md new file mode 100644 index 00000000000..5a898777775 --- /dev/null +++ b/.changeset/brown-hotels-rush.md @@ -0,0 +1,5 @@ +--- +"@remix-run/dev": patch +--- + +Bundle CSS imported in client entry file in Vite plugin diff --git a/integration/vite-css-build-test.ts b/integration/vite-css-build-test.ts index 8ebc1f65391..46b917a6c5e 100644 --- a/integration/vite-css-build-test.ts +++ b/integration/vite-css-build-test.ts @@ -50,6 +50,22 @@ test.describe("Vite CSS build", () => { ], }); `, + "app/entry.client.tsx": js` + import "./entry.client.css"; + + import { RemixBrowser } from "@remix-run/react"; + import { startTransition, StrictMode } from "react"; + import { hydrateRoot } from "react-dom/client"; + + startTransition(() => { + hydrateRoot( + document, + + + + ); + }); + `, "app/root.tsx": js` import { Links, Meta, Outlet, Scripts } from "@remix-run/react"; @@ -70,6 +86,12 @@ test.describe("Vite CSS build", () => { ); } `, + "app/entry.client.css": css` + .client-entry { + background: pink; + padding: ${TEST_PADDING_VALUE}; + } + `, "app/routes/_index/styles-bundled.css": css` .index_bundled { background: papayawhip; @@ -118,12 +140,14 @@ test.describe("Vite CSS build", () => { export default function IndexRoute() { return (
-
-
-
-
-
-

CSS test

+
+
+
+
+
+
+

CSS test

+
@@ -146,6 +170,10 @@ test.describe("Vite CSS build", () => { test("renders styles", async ({ page }) => { let app = new PlaywrightFixture(appFixture, page); await app.goto("/"); + await expect(page.locator("#index [data-client-entry]")).toHaveCSS( + "padding", + TEST_PADDING_VALUE + ); await expect(page.locator("#index [data-css-modules]")).toHaveCSS( "padding", TEST_PADDING_VALUE diff --git a/integration/vite-css-dev-test.ts b/integration/vite-css-dev-test.ts index 15e30295950..331f9d00cca 100644 --- a/integration/vite-css-dev-test.ts +++ b/integration/vite-css-dev-test.ts @@ -43,6 +43,22 @@ test.describe("Vite CSS dev", () => { ], }); `, + "app/entry.client.tsx": js` + import "./entry.client.css"; + + import { RemixBrowser } from "@remix-run/react"; + import { startTransition, StrictMode } from "react"; + import { hydrateRoot } from "react-dom/client"; + + startTransition(() => { + hydrateRoot( + document, + + + + ); + }); + `, "app/root.tsx": js` import { Links, Meta, Outlet, Scripts, LiveReload } from "@remix-run/react"; @@ -64,6 +80,12 @@ test.describe("Vite CSS dev", () => { ); } `, + "app/entry.client.css": css` + .client-entry { + background: pink; + padding: ${TEST_PADDING_VALUE}; + } + `, "app/styles-bundled.css": css` .index_bundled { background: papayawhip; @@ -113,12 +135,14 @@ test.describe("Vite CSS dev", () => { return (
-
-
-
-
-
-

CSS test

+
+
+
+
+
+
+

CSS test

+
@@ -169,6 +193,10 @@ test.describe("Vite CSS dev", () => { await page.goto(`http://localhost:${devPort}/`, { waitUntil: "networkidle", }); + await expect(page.locator("#index [data-client-entry]")).toHaveCSS( + "padding", + TEST_PADDING_VALUE + ); await expect(page.locator("#index [data-css-modules]")).toHaveCSS( "padding", TEST_PADDING_VALUE @@ -205,6 +233,10 @@ test.describe("Vite CSS dev", () => { // Ensure no errors on page load expect(pageErrors).toEqual([]); + await expect(page.locator("#index [data-client-entry]")).toHaveCSS( + "padding", + TEST_PADDING_VALUE + ); await expect(page.locator("#index [data-css-modules]")).toHaveCSS( "padding", TEST_PADDING_VALUE @@ -300,8 +332,28 @@ test.describe("Vite CSS dev", () => { UPDATED_TEST_PADDING_VALUE ); + // Ensure CSS updates were handled by HMR await expect(input).toHaveValue("stateful"); + // The following change triggers a full page reload, so we check it after all the checks for HMR state preservation + let clientEntryCssContents = await fs.readFile( + path.join(projectDir, "app/entry.client.css"), + "utf8" + ); + await fs.writeFile( + path.join(projectDir, "app/entry.client.css"), + clientEntryCssContents.replace( + TEST_PADDING_VALUE, + UPDATED_TEST_PADDING_VALUE + ), + "utf8" + ); + + await expect(page.locator("#index [data-client-entry]")).toHaveCSS( + "padding", + UPDATED_TEST_PADDING_VALUE + ); + expect(pageErrors).toEqual([]); }); }); diff --git a/packages/remix-dev/vite/import-vite-esm-sync.ts b/packages/remix-dev/vite/import-vite-esm-sync.ts new file mode 100644 index 00000000000..f48894cdbaf --- /dev/null +++ b/packages/remix-dev/vite/import-vite-esm-sync.ts @@ -0,0 +1,21 @@ +// This file is used to avoid CJS deprecation warnings in Vite 5 since +// @remix-run/dev currently compiles to CJS. By using this interface, we only +// ever access the Vite package via a dynamic import which forces the ESM build. +// "importViteAsync" is expected be called up-front in the first async plugin +// hook, which then unlocks "importViteEsmSync" for use anywhere in the plugin +// and its utils. This file won't be needed when this package is ESM only. + +import invariant from "../invariant"; + +// eslint-disable-next-line @typescript-eslint/consistent-type-imports +type Vite = typeof import("vite"); +let vite: Vite | undefined; + +export async function preloadViteEsm(): Promise { + vite = await import("vite"); +} + +export function importViteEsmSync(): Vite { + invariant(vite, "importViteEsmSync() called before preloadViteEsm()"); + return vite; +} diff --git a/packages/remix-dev/vite/plugin.ts b/packages/remix-dev/vite/plugin.ts index fa1b001d15f..b8ffd45b909 100644 --- a/packages/remix-dev/vite/plugin.ts +++ b/packages/remix-dev/vite/plugin.ts @@ -29,13 +29,10 @@ import { createRequestHandler } from "./node/adapter"; import { getStylesForUrl, isCssModulesFile } from "./styles"; import * as VirtualModule from "./vmod"; import { serverEntryId } from "./server-entry-id"; +import { resolveFileUrl } from "./resolve-file-url"; import { removeExports } from "./remove-exports"; import { replaceImportSpecifier } from "./replace-import-specifier"; - -// We reassign the "vite" variable from a dynamic import of Vite's ESM build -// when the Vite plugin's config hook is executed -// eslint-disable-next-line @typescript-eslint/consistent-type-imports -let vite: typeof import("vite"); +import { importViteEsmSync, preloadViteEsm } from "./import-vite-esm-sync"; const supportedRemixConfigKeys = [ "appDirectory", @@ -95,24 +92,6 @@ let remixReactProxyId = VirtualModule.id("remix-react-proxy"); let hmrRuntimeId = VirtualModule.id("hmr-runtime"); let injectHmrRuntimeId = VirtualModule.id("inject-hmr-runtime"); -const resolveFileUrl = ( - { rootDirectory }: Pick, - filePath: string -) => { - let relativePath = path.relative(rootDirectory, filePath); - let isWithinRoot = - !relativePath.startsWith("..") && !path.isAbsolute(relativePath); - - if (!isWithinRoot) { - // Vite will prevent serving files outside of the workspace - // unless user explictly opts in with `server.fs.allow` - // https://vitejs.dev/config/server-options.html#server-fs-allow - return path.posix.join("/@fs", vite.normalizePath(filePath)); - } - - return "/" + vite.normalizePath(relativePath); -}; - const isJsFile = (filePath: string) => /\.[cm]?[jt]sx?$/i.test(filePath); type Route = RouteManifest[string]; @@ -120,6 +99,7 @@ const resolveRelativeRouteFilePath = ( route: Route, pluginConfig: ResolvedRemixVitePluginConfig ) => { + let vite = importViteEsmSync(); let file = route.file; let fullPath = path.resolve(pluginConfig.appDirectory, file); @@ -133,11 +113,12 @@ const getHash = (source: BinaryLike, maxLength?: number): string => { return typeof maxLength === "number" ? hash.slice(0, maxLength) : hash; }; -const resolveBuildAssetPaths = ( +const resolveChunk = ( pluginConfig: ResolvedRemixVitePluginConfig, viteManifest: Vite.Manifest, absoluteFilePath: string -): Manifest["entry"] & { css: string[] } => { +) => { + let vite = importViteEsmSync(); let rootRelativeFilePath = path.relative( pluginConfig.rootDirectory, absoluteFilePath @@ -154,7 +135,26 @@ const resolveBuildAssetPaths = ( ); } - let chunks = resolveDependantChunks(viteManifest, entryChunk); + return entryChunk; +}; + +const resolveBuildAssetPaths = ( + pluginConfig: ResolvedRemixVitePluginConfig, + viteManifest: Vite.Manifest, + entryFilePath: string, + prependedAssetFilePaths: string[] = [] +): Manifest["entry"] & { css: string[] } => { + let entryChunk = resolveChunk(pluginConfig, viteManifest, entryFilePath); + + // This is here to support prepending client entry assets to the root route + let prependedAssetChunks = prependedAssetFilePaths.map((filePath) => + resolveChunk(pluginConfig, viteManifest, filePath) + ); + + let chunks = resolveDependantChunks(viteManifest, [ + ...prependedAssetChunks, + entryChunk, + ]); return { module: `${pluginConfig.publicPath}${entryChunk.file}`, @@ -171,7 +171,7 @@ const resolveBuildAssetPaths = ( function resolveDependantChunks( viteManifest: Vite.Manifest, - entryChunk: Vite.ManifestChunk + entryChunks: Vite.ManifestChunk[] ): Vite.ManifestChunk[] { let chunks = new Set(); @@ -189,7 +189,9 @@ function resolveDependantChunks( chunks.add(chunk); } - walk(entryChunk); + for (let entryChunk of entryChunks) { + walk(entryChunk); + } return Array.from(chunks); } @@ -400,7 +402,7 @@ export const remixVitePlugin: RemixVitePlugin = (options = {}) => { pluginConfig.assetsBuildDirectory ); - let entry: Manifest["entry"] = resolveBuildAssetPaths( + let entry = resolveBuildAssetPaths( pluginConfig, viteManifest, pluginConfig.entryClientFilePath @@ -416,6 +418,7 @@ export const remixVitePlugin: RemixVitePlugin = (options = {}) => { for (let [key, route] of Object.entries(pluginConfig.routes)) { let routeFilePath = path.join(pluginConfig.appDirectory, route.file); let sourceExports = routeManifestExports[key]; + let isRootRoute = route.parentId === undefined; routes[key] = { id: route.id, @@ -426,7 +429,15 @@ export const remixVitePlugin: RemixVitePlugin = (options = {}) => { hasAction: sourceExports.includes("action"), hasLoader: sourceExports.includes("loader"), hasErrorBoundary: sourceExports.includes("ErrorBoundary"), - ...resolveBuildAssetPaths(pluginConfig, viteManifest, routeFilePath), + ...resolveBuildAssetPaths( + pluginConfig, + viteManifest, + routeFilePath, + // If this is the root route, we also need to include assets from the + // client entry file as this is a common way for consumers to import + // global reset styles, etc. + isRootRoute ? [pluginConfig.entryClientFilePath] : [] + ), }; } @@ -497,8 +508,11 @@ export const remixVitePlugin: RemixVitePlugin = (options = {}) => { { name: "remix", config: async (_viteUserConfig, viteConfigEnv) => { - // Load Vite's ESM build up-front as soon as we're in an async context - vite = await import("vite"); + // Preload Vite's ESM build up-front as soon as we're in an async context + await preloadViteEsm(); + + // Ensure sync import of Vite works after async preload + let vite = importViteEsmSync(); viteUserConfig = _viteUserConfig; viteCommand = viteConfigEnv.command; @@ -627,6 +641,9 @@ export const remixVitePlugin: RemixVitePlugin = (options = {}) => { "The Remix Vite plugin requires the use of a Vite config file" ); } + + let vite = importViteEsmSync(); + let childCompilerConfigFile = await vite.loadConfigFromFile( { command: viteConfig.command, @@ -684,8 +701,8 @@ export const remixVitePlugin: RemixVitePlugin = (options = {}) => { showUnstableWarning(); } }, - configureServer(vite) { - vite.httpServer?.on("listening", () => { + configureServer(viteDevServer) { + viteDevServer.httpServer?.on("listening", () => { setTimeout(showUnstableWarning, 50); }); @@ -695,7 +712,7 @@ export const remixVitePlugin: RemixVitePlugin = (options = {}) => { getCriticalCss: async (build, url) => { invariant(cachedPluginConfig); return getStylesForUrl( - vite, + viteDevServer, cachedPluginConfig, cssModulesManifest, build, @@ -706,7 +723,7 @@ export const remixVitePlugin: RemixVitePlugin = (options = {}) => { // stack trace so it maps back to the actual source code processRequestError: (error) => { if (error instanceof Error) { - vite.ssrFixStacktrace(error); + viteDevServer.ssrFixStacktrace(error); } }, }); @@ -718,7 +735,7 @@ export const remixVitePlugin: RemixVitePlugin = (options = {}) => { let previousPluginConfig: ResolvedRemixVitePluginConfig | undefined; return () => { - vite.middlewares.use(async (_req, _res, next) => { + viteDevServer.middlewares.use(async (_req, _res, next) => { try { let pluginConfig = await resolvePluginConfig(); @@ -730,12 +747,12 @@ export const remixVitePlugin: RemixVitePlugin = (options = {}) => { // Invalidate all virtual modules vmods.forEach((vmod) => { - let mod = vite.moduleGraph.getModuleById( + let mod = viteDevServer.moduleGraph.getModuleById( VirtualModule.resolve(vmod) ); if (mod) { - vite.moduleGraph.invalidateModule(mod); + viteDevServer.moduleGraph.invalidateModule(mod); } }); } @@ -748,10 +765,10 @@ export const remixVitePlugin: RemixVitePlugin = (options = {}) => { // Let user servers handle SSR requests in middleware mode, // otherwise the Vite plugin will handle the request - if (!vite.config.server.middlewareMode) { - vite.middlewares.use(async (req, res, next) => { + if (!viteDevServer.config.server.middlewareMode) { + viteDevServer.middlewares.use(async (req, res, next) => { try { - let build = (await vite.ssrLoadModule( + let build = (await viteDevServer.ssrLoadModule( serverEntryId )) as ServerBuild; @@ -1151,6 +1168,7 @@ function getRoute( pluginConfig: ResolvedRemixVitePluginConfig, file: string ): Route | undefined { + let vite = importViteEsmSync(); if (!file.startsWith(vite.normalizePath(pluginConfig.appDirectory))) return; let routePath = vite.normalizePath( path.relative(pluginConfig.appDirectory, file) diff --git a/packages/remix-dev/vite/resolve-file-url.ts b/packages/remix-dev/vite/resolve-file-url.ts new file mode 100644 index 00000000000..37482705f5b --- /dev/null +++ b/packages/remix-dev/vite/resolve-file-url.ts @@ -0,0 +1,22 @@ +import * as path from "node:path"; + +import { importViteEsmSync } from "./import-vite-esm-sync"; + +export const resolveFileUrl = ( + { rootDirectory }: { rootDirectory: string }, + filePath: string +) => { + let vite = importViteEsmSync(); + let relativePath = path.relative(rootDirectory, filePath); + let isWithinRoot = + !relativePath.startsWith("..") && !path.isAbsolute(relativePath); + + if (!isWithinRoot) { + // Vite will prevent serving files outside of the workspace + // unless user explictly opts in with `server.fs.allow` + // https://vitejs.dev/config/server-options.html#server-fs-allow + return path.posix.join("/@fs", vite.normalizePath(filePath)); + } + + return "/" + vite.normalizePath(relativePath); +}; diff --git a/packages/remix-dev/vite/styles.ts b/packages/remix-dev/vite/styles.ts index c2421a545ee..dfe8406a1f7 100644 --- a/packages/remix-dev/vite/styles.ts +++ b/packages/remix-dev/vite/styles.ts @@ -4,6 +4,7 @@ import { matchRoutes } from "@remix-run/router"; import { type ModuleNode, type ViteDevServer } from "vite"; import { type RemixConfig as ResolvedRemixConfig } from "../config"; +import { resolveFileUrl } from "./resolve-file-url"; type ServerRouteManifest = ServerBuild["routes"]; type ServerRoute = ServerRouteManifest[string]; @@ -21,7 +22,8 @@ const isCssFile = (file: string) => cssFileRegExp.test(file); export const isCssModulesFile = (file: string) => cssModulesRegExp.test(file); const getStylesForFiles = async ( - viteServer: ViteDevServer, + viteDevServer: ViteDevServer, + config: { rootDirectory: string }, cssModulesManifest: Record, files: string[] ): Promise => { @@ -31,22 +33,31 @@ const getStylesForFiles = async ( try { for (let file of files) { let normalizedPath = path.resolve(file).replace(/\\/g, "/"); - let node = await viteServer.moduleGraph.getModuleById(normalizedPath); - if (!node) { - let absolutePath = path.resolve(file); - await viteServer.ssrLoadModule(absolutePath); - node = await viteServer.moduleGraph.getModuleByUrl(absolutePath); + let node = await viteDevServer.moduleGraph.getModuleById(normalizedPath); - if (!node) { - console.log(`Could not resolve module for file: ${file}`); - continue; + // If the module is only present in the client module graph, the module + // won't have been found on the first request to the server. If so, we + // request the module so it's in the module graph, then try again. + if (!node) { + try { + await viteDevServer.transformRequest( + resolveFileUrl(config, normalizedPath) + ); + } catch (err) { + console.error(err); } + node = await viteDevServer.moduleGraph.getModuleById(normalizedPath); + } + + if (!node) { + console.log(`Could not resolve module for file: ${file}`); + continue; } - await findDeps(viteServer, node, deps); + await findDeps(viteDevServer, node, deps); } - } catch (e) { - console.error(e); + } catch (err) { + console.error(err); } for (let dep of deps) { @@ -58,7 +69,7 @@ const getStylesForFiles = async ( try { let css = isCssModulesFile(dep.file) ? cssModulesManifest[dep.file] - : (await viteServer.ssrLoadModule(dep.url)).default; + : (await viteDevServer.ssrLoadModule(dep.url)).default; if (css === undefined) { throw new Error(); @@ -156,8 +167,11 @@ const createRoutes = ( }; export const getStylesForUrl = async ( - vite: ViteDevServer, - config: Pick, + viteDevServer: ViteDevServer, + config: Pick< + ResolvedRemixConfig, + "appDirectory" | "routes" | "rootDirectory" | "entryClientFilePath" + >, cssModulesManifest: Record, build: ServerBuild, url: string | undefined @@ -174,9 +188,15 @@ export const getStylesForUrl = async ( ) ?? []; let styles = await getStylesForFiles( - vite, + viteDevServer, + config, cssModulesManifest, - documentRouteFiles + [ + // Always include the client entry file when crawling the module graph for CSS + path.relative(config.rootDirectory, config.entryClientFilePath), + // Then include any styles from the matched routes + ...documentRouteFiles, + ] ); return styles;