From 54223b7b2ef6c4048918e2f70a932562d5d7fcdd Mon Sep 17 00:00:00 2001 From: Mark Dalgleish Date: Mon, 27 Nov 2023 14:49:11 +1100 Subject: [PATCH 1/6] fix(remix-dev/vite): bundle CSS from client entry --- .changeset/brown-hotels-rush.md | 5 ++ integration/vite-css-build-test.ts | 40 +++++++-- integration/vite-css-dev-test.ts | 62 ++++++++++++-- packages/remix-dev/vite/plugin.ts | 92 ++++++++++++--------- packages/remix-dev/vite/resolve-file-url.ts | 21 +++++ packages/remix-dev/vite/styles.ts | 53 +++++++++--- 6 files changed, 213 insertions(+), 60 deletions(-) create mode 100644 .changeset/brown-hotels-rush.md 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..6cfd2f93b10 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 @@ -231,6 +263,19 @@ test.describe("Vite CSS dev", () => { await input.type("stateful"); await expect(input).toHaveValue("stateful"); + 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" + ); + let bundledCssContents = await fs.readFile( path.join(projectDir, "app/styles-bundled.css"), "utf8" @@ -279,6 +324,11 @@ test.describe("Vite CSS dev", () => { (data) => data.replace(TEST_PADDING_VALUE, UPDATED_TEST_PADDING_VALUE) ); + await expect(page.locator("#index [data-client-entry]")).toHaveCSS( + "padding", + UPDATED_TEST_PADDING_VALUE + ); + await expect(page.locator("#index [data-css-modules]")).toHaveCSS( "padding", UPDATED_TEST_PADDING_VALUE diff --git a/packages/remix-dev/vite/plugin.ts b/packages/remix-dev/vite/plugin.ts index f5ad61e3c50..ff1ca1beddb 100644 --- a/packages/remix-dev/vite/plugin.ts +++ b/packages/remix-dev/vite/plugin.ts @@ -29,6 +29,7 @@ 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"; @@ -95,24 +96,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]; @@ -133,11 +116,11 @@ 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 rootRelativeFilePath = path.relative( pluginConfig.rootDirectory, absoluteFilePath @@ -154,7 +137,24 @@ const resolveBuildAssetPaths = ( ); } - let chunks = resolveDependantChunks(viteManifest, entryChunk); + return entryChunk; +}; + +const resolveBuildAssetPaths = ( + pluginConfig: ResolvedRemixVitePluginConfig, + viteManifest: Vite.Manifest, + entryFilePath: string, + includedAssetFilePaths: string[] = [] +): Manifest["entry"] & { css: string[] } => { + let entryChunk = resolveChunk(pluginConfig, viteManifest, entryFilePath); + let includedAssetChunks = includedAssetFilePaths.map((filePath) => + resolveChunk(pluginConfig, viteManifest, filePath) + ); + + let chunks = resolveDependantChunks(viteManifest, [ + entryChunk, + ...includedAssetChunks, + ]); 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); } @@ -220,7 +222,7 @@ const getRouteModuleExports = async ( let ssr = true; let { pluginContainer, moduleGraph } = viteChildCompiler; let routePath = path.join(pluginConfig.appDirectory, routeFile); - let url = resolveFileUrl(pluginConfig, routePath); + let url = resolveFileUrl(vite, pluginConfig, routePath); let resolveId = async () => { let result = await pluginContainer.resolveId(url, undefined, { ssr }); @@ -324,13 +326,14 @@ export const remixVitePlugin: RemixVitePlugin = (options = {}) => { return ` import * as entryServer from ${JSON.stringify( - resolveFileUrl(pluginConfig, pluginConfig.entryServerFilePath) + resolveFileUrl(vite, pluginConfig, pluginConfig.entryServerFilePath) )}; ${Object.keys(pluginConfig.routes) .map((key, index) => { let route = pluginConfig.routes[key]!; return `import * as route${index} from ${JSON.stringify( resolveFileUrl( + vite, pluginConfig, resolveRelativeRouteFilePath(route, pluginConfig) ) @@ -383,7 +386,7 @@ export const remixVitePlugin: RemixVitePlugin = (options = {}) => { pluginConfig.assetsBuildDirectory ); - let entry: Manifest["entry"] = resolveBuildAssetPaths( + let entry = resolveBuildAssetPaths( pluginConfig, viteManifest, pluginConfig.entryClientFilePath @@ -407,7 +410,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. + route.parentId === undefined ? [pluginConfig.entryClientFilePath] : [] + ), }; } @@ -448,6 +459,7 @@ export const remixVitePlugin: RemixVitePlugin = (options = {}) => { index: route.index, caseSensitive: route.caseSensitive, module: `${resolveFileUrl( + vite, pluginConfig, resolveRelativeRouteFilePath(route, pluginConfig) )}${ @@ -467,7 +479,11 @@ export const remixVitePlugin: RemixVitePlugin = (options = {}) => { runtime: VirtualModule.url(injectHmrRuntimeId), }, entry: { - module: resolveFileUrl(pluginConfig, pluginConfig.entryClientFilePath), + module: resolveFileUrl( + vite, + pluginConfig, + pluginConfig.entryClientFilePath + ), imports: [], }, routes, @@ -665,8 +681,8 @@ export const remixVitePlugin: RemixVitePlugin = (options = {}) => { showUnstableWarning(); } }, - configureServer(vite) { - vite.httpServer?.on("listening", () => { + configureServer(viteDevServer) { + viteDevServer.httpServer?.on("listening", () => { setTimeout(showUnstableWarning, 50); }); @@ -677,6 +693,7 @@ export const remixVitePlugin: RemixVitePlugin = (options = {}) => { invariant(cachedPluginConfig); return getStylesForUrl( vite, + viteDevServer, cachedPluginConfig, cssModulesManifest, build, @@ -687,7 +704,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); } }, }); @@ -699,7 +716,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(); @@ -711,12 +728,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); } }); } @@ -729,10 +746,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; @@ -1158,6 +1175,7 @@ async function getRouteMetadata( resolveRelativeRouteFilePath(route, pluginConfig) ), module: `${resolveFileUrl( + vite, pluginConfig, resolveRelativeRouteFilePath(route, pluginConfig) )}?import`, // Ensure the Vite dev server responds with a JS module 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..4f258b1c3ca --- /dev/null +++ b/packages/remix-dev/vite/resolve-file-url.ts @@ -0,0 +1,21 @@ +import * as path from "node:path"; + +export const resolveFileUrl = ( + // eslint-disable-next-line @typescript-eslint/consistent-type-imports + vite: typeof import("vite"), + { rootDirectory }: { rootDirectory: string }, + 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); +}; diff --git a/packages/remix-dev/vite/styles.ts b/packages/remix-dev/vite/styles.ts index c2421a545ee..2b84d34dcf7 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,10 @@ const isCssFile = (file: string) => cssFileRegExp.test(file); export const isCssModulesFile = (file: string) => cssModulesRegExp.test(file); const getStylesForFiles = async ( - viteServer: ViteDevServer, + // eslint-disable-next-line @typescript-eslint/consistent-type-imports + vite: typeof import("vite"), + viteDevServer: ViteDevServer, + config: { rootDirectory: string }, cssModulesManifest: Record, files: string[] ): Promise => { @@ -31,11 +35,26 @@ const getStylesForFiles = async ( try { for (let file of files) { let normalizedPath = path.resolve(file).replace(/\\/g, "/"); - let node = await viteServer.moduleGraph.getModuleById(normalizedPath); + let node = await viteDevServer.moduleGraph.getModuleById(normalizedPath); + + // 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(vite, config, normalizedPath) + ); + } catch (err) { + console.error(err); + } + node = await viteDevServer.moduleGraph.getModuleById(normalizedPath); + } + if (!node) { let absolutePath = path.resolve(file); - await viteServer.ssrLoadModule(absolutePath); - node = await viteServer.moduleGraph.getModuleByUrl(absolutePath); + await viteDevServer.ssrLoadModule(absolutePath); + node = await viteDevServer.moduleGraph.getModuleByUrl(absolutePath); if (!node) { console.log(`Could not resolve module for file: ${file}`); @@ -43,10 +62,10 @@ const getStylesForFiles = async ( } } - 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 +77,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 +175,13 @@ const createRoutes = ( }; export const getStylesForUrl = async ( - vite: ViteDevServer, - config: Pick, + // eslint-disable-next-line @typescript-eslint/consistent-type-imports + vite: typeof import("vite"), + viteDevServer: ViteDevServer, + config: Pick< + ResolvedRemixConfig, + "appDirectory" | "routes" | "rootDirectory" | "entryClientFilePath" + >, cssModulesManifest: Record, build: ServerBuild, url: string | undefined @@ -175,8 +199,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; From d37437dab2773f069d78047a32bf083d2e5a56d0 Mon Sep 17 00:00:00 2001 From: Mark Dalgleish Date: Tue, 28 Nov 2023 06:05:50 +1100 Subject: [PATCH 2/6] fix test --- integration/vite-css-dev-test.ts | 40 ++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/integration/vite-css-dev-test.ts b/integration/vite-css-dev-test.ts index 6cfd2f93b10..b61e09aba06 100644 --- a/integration/vite-css-dev-test.ts +++ b/integration/vite-css-dev-test.ts @@ -263,19 +263,6 @@ test.describe("Vite CSS dev", () => { await input.type("stateful"); await expect(input).toHaveValue("stateful"); - 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" - ); - let bundledCssContents = await fs.readFile( path.join(projectDir, "app/styles-bundled.css"), "utf8" @@ -324,11 +311,6 @@ test.describe("Vite CSS dev", () => { (data) => data.replace(TEST_PADDING_VALUE, UPDATED_TEST_PADDING_VALUE) ); - await expect(page.locator("#index [data-client-entry]")).toHaveCSS( - "padding", - UPDATED_TEST_PADDING_VALUE - ); - await expect(page.locator("#index [data-css-modules]")).toHaveCSS( "padding", UPDATED_TEST_PADDING_VALUE @@ -350,8 +332,30 @@ test.describe("Vite CSS dev", () => { UPDATED_TEST_PADDING_VALUE ); + // Ensure CSS updates were handled by HMR await expect(input).toHaveValue("stateful"); + 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 + ); + + // Changes to the client entry fall back to a full page reload + await expect(input).toHaveValue(""); + expect(pageErrors).toEqual([]); }); }); From 83b40faabd52371eb4d6267a63c18edb88c2ad48 Mon Sep 17 00:00:00 2001 From: Mark Dalgleish Date: Tue, 28 Nov 2023 07:06:57 +1100 Subject: [PATCH 3/6] remove state check --- integration/vite-css-dev-test.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/integration/vite-css-dev-test.ts b/integration/vite-css-dev-test.ts index b61e09aba06..26a8847a6ff 100644 --- a/integration/vite-css-dev-test.ts +++ b/integration/vite-css-dev-test.ts @@ -222,8 +222,8 @@ test.describe("Vite CSS dev", () => { test.describe("with JS", () => { test.use({ javaScriptEnabled: true }); - test("updates CSS", async ({ page }) => { - let pageErrors: unknown[] = []; + test("updates CSS", async ({ page, browserName }) => { + let pageErrors: Error[] = []; page.on("pageerror", (error) => pageErrors.push(error)); await page.goto(`http://localhost:${devPort}/`, { @@ -335,6 +335,7 @@ test.describe("Vite CSS dev", () => { // 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 the HMR changes let clientEntryCssContents = await fs.readFile( path.join(projectDir, "app/entry.client.css"), "utf8" @@ -353,9 +354,6 @@ test.describe("Vite CSS dev", () => { UPDATED_TEST_PADDING_VALUE ); - // Changes to the client entry fall back to a full page reload - await expect(input).toHaveValue(""); - expect(pageErrors).toEqual([]); }); }); From 26c66c3361914b7c2e8659e05e0cfb17cf6aec0f Mon Sep 17 00:00:00 2001 From: Mark Dalgleish Date: Tue, 28 Nov 2023 07:51:57 +1100 Subject: [PATCH 4/6] revert stray test updates --- integration/vite-css-dev-test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/integration/vite-css-dev-test.ts b/integration/vite-css-dev-test.ts index 26a8847a6ff..e9339bc9021 100644 --- a/integration/vite-css-dev-test.ts +++ b/integration/vite-css-dev-test.ts @@ -222,8 +222,8 @@ test.describe("Vite CSS dev", () => { test.describe("with JS", () => { test.use({ javaScriptEnabled: true }); - test("updates CSS", async ({ page, browserName }) => { - let pageErrors: Error[] = []; + test("updates CSS", async ({ page }) => { + let pageErrors: unknown[] = []; page.on("pageerror", (error) => pageErrors.push(error)); await page.goto(`http://localhost:${devPort}/`, { From 26344e782bcc62ead73a7dcab83d54c45175bb4d Mon Sep 17 00:00:00 2001 From: Mark Dalgleish Date: Tue, 28 Nov 2023 11:17:40 +1100 Subject: [PATCH 5/6] code review and refactor --- .../remix-dev/vite/import-vite-esm-sync.ts | 21 +++++++++ packages/remix-dev/vite/plugin.ts | 44 +++++++++---------- packages/remix-dev/vite/resolve-file-url.ts | 5 ++- packages/remix-dev/vite/styles.ts | 17 ++----- 4 files changed, 49 insertions(+), 38 deletions(-) create mode 100644 packages/remix-dev/vite/import-vite-esm-sync.ts 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 19407c7c925..b8ffd45b909 100644 --- a/packages/remix-dev/vite/plugin.ts +++ b/packages/remix-dev/vite/plugin.ts @@ -32,11 +32,7 @@ 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", @@ -103,6 +99,7 @@ const resolveRelativeRouteFilePath = ( route: Route, pluginConfig: ResolvedRemixVitePluginConfig ) => { + let vite = importViteEsmSync(); let file = route.file; let fullPath = path.resolve(pluginConfig.appDirectory, file); @@ -121,6 +118,7 @@ const resolveChunk = ( viteManifest: Vite.Manifest, absoluteFilePath: string ) => { + let vite = importViteEsmSync(); let rootRelativeFilePath = path.relative( pluginConfig.rootDirectory, absoluteFilePath @@ -144,16 +142,18 @@ const resolveBuildAssetPaths = ( pluginConfig: ResolvedRemixVitePluginConfig, viteManifest: Vite.Manifest, entryFilePath: string, - includedAssetFilePaths: string[] = [] + prependedAssetFilePaths: string[] = [] ): Manifest["entry"] & { css: string[] } => { let entryChunk = resolveChunk(pluginConfig, viteManifest, entryFilePath); - let includedAssetChunks = includedAssetFilePaths.map((filePath) => + + // 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, - ...includedAssetChunks, ]); return { @@ -239,7 +239,7 @@ const getRouteModuleExports = async ( let ssr = true; let { pluginContainer, moduleGraph } = viteChildCompiler; let routePath = path.join(pluginConfig.appDirectory, routeFile); - let url = resolveFileUrl(vite, pluginConfig, routePath); + let url = resolveFileUrl(pluginConfig, routePath); let resolveId = async () => { let result = await pluginContainer.resolveId(url, undefined, { ssr }); @@ -343,14 +343,13 @@ export const remixVitePlugin: RemixVitePlugin = (options = {}) => { return ` import * as entryServer from ${JSON.stringify( - resolveFileUrl(vite, pluginConfig, pluginConfig.entryServerFilePath) + resolveFileUrl(pluginConfig, pluginConfig.entryServerFilePath) )}; ${Object.keys(pluginConfig.routes) .map((key, index) => { let route = pluginConfig.routes[key]!; return `import * as route${index} from ${JSON.stringify( resolveFileUrl( - vite, pluginConfig, resolveRelativeRouteFilePath(route, pluginConfig) ) @@ -419,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, @@ -436,7 +436,7 @@ export const remixVitePlugin: RemixVitePlugin = (options = {}) => { // 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. - route.parentId === undefined ? [pluginConfig.entryClientFilePath] : [] + isRootRoute ? [pluginConfig.entryClientFilePath] : [] ), }; } @@ -478,7 +478,6 @@ export const remixVitePlugin: RemixVitePlugin = (options = {}) => { index: route.index, caseSensitive: route.caseSensitive, module: `${resolveFileUrl( - vite, pluginConfig, resolveRelativeRouteFilePath(route, pluginConfig) )}${ @@ -498,11 +497,7 @@ export const remixVitePlugin: RemixVitePlugin = (options = {}) => { runtime: VirtualModule.url(injectHmrRuntimeId), }, entry: { - module: resolveFileUrl( - vite, - pluginConfig, - pluginConfig.entryClientFilePath - ), + module: resolveFileUrl(pluginConfig, pluginConfig.entryClientFilePath), imports: [], }, routes, @@ -513,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; @@ -643,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, @@ -711,7 +712,6 @@ export const remixVitePlugin: RemixVitePlugin = (options = {}) => { getCriticalCss: async (build, url) => { invariant(cachedPluginConfig); return getStylesForUrl( - vite, viteDevServer, cachedPluginConfig, cssModulesManifest, @@ -1168,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) @@ -1202,7 +1203,6 @@ async function getRouteMetadata( resolveRelativeRouteFilePath(route, pluginConfig) ), module: `${resolveFileUrl( - vite, pluginConfig, resolveRelativeRouteFilePath(route, pluginConfig) )}?import`, // Ensure the Vite dev server responds with a JS module diff --git a/packages/remix-dev/vite/resolve-file-url.ts b/packages/remix-dev/vite/resolve-file-url.ts index 4f258b1c3ca..37482705f5b 100644 --- a/packages/remix-dev/vite/resolve-file-url.ts +++ b/packages/remix-dev/vite/resolve-file-url.ts @@ -1,11 +1,12 @@ import * as path from "node:path"; +import { importViteEsmSync } from "./import-vite-esm-sync"; + export const resolveFileUrl = ( - // eslint-disable-next-line @typescript-eslint/consistent-type-imports - vite: typeof import("vite"), { rootDirectory }: { rootDirectory: string }, filePath: string ) => { + let vite = importViteEsmSync(); let relativePath = path.relative(rootDirectory, filePath); let isWithinRoot = !relativePath.startsWith("..") && !path.isAbsolute(relativePath); diff --git a/packages/remix-dev/vite/styles.ts b/packages/remix-dev/vite/styles.ts index 2b84d34dcf7..dfe8406a1f7 100644 --- a/packages/remix-dev/vite/styles.ts +++ b/packages/remix-dev/vite/styles.ts @@ -22,8 +22,6 @@ const isCssFile = (file: string) => cssFileRegExp.test(file); export const isCssModulesFile = (file: string) => cssModulesRegExp.test(file); const getStylesForFiles = async ( - // eslint-disable-next-line @typescript-eslint/consistent-type-imports - vite: typeof import("vite"), viteDevServer: ViteDevServer, config: { rootDirectory: string }, cssModulesManifest: Record, @@ -43,7 +41,7 @@ const getStylesForFiles = async ( if (!node) { try { await viteDevServer.transformRequest( - resolveFileUrl(vite, config, normalizedPath) + resolveFileUrl(config, normalizedPath) ); } catch (err) { console.error(err); @@ -52,14 +50,8 @@ const getStylesForFiles = async ( } if (!node) { - let absolutePath = path.resolve(file); - await viteDevServer.ssrLoadModule(absolutePath); - node = await viteDevServer.moduleGraph.getModuleByUrl(absolutePath); - - if (!node) { - console.log(`Could not resolve module for file: ${file}`); - continue; - } + console.log(`Could not resolve module for file: ${file}`); + continue; } await findDeps(viteDevServer, node, deps); @@ -175,8 +167,6 @@ const createRoutes = ( }; export const getStylesForUrl = async ( - // eslint-disable-next-line @typescript-eslint/consistent-type-imports - vite: typeof import("vite"), viteDevServer: ViteDevServer, config: Pick< ResolvedRemixConfig, @@ -198,7 +188,6 @@ export const getStylesForUrl = async ( ) ?? []; let styles = await getStylesForFiles( - vite, viteDevServer, config, cssModulesManifest, From 25a7e9290bcd649af0747f6ca4f207a6958ef17f Mon Sep 17 00:00:00 2001 From: Mark Dalgleish Date: Tue, 28 Nov 2023 12:03:49 +1100 Subject: [PATCH 6/6] tweak comment Co-authored-by: Pedro Cattori --- integration/vite-css-dev-test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration/vite-css-dev-test.ts b/integration/vite-css-dev-test.ts index e9339bc9021..331f9d00cca 100644 --- a/integration/vite-css-dev-test.ts +++ b/integration/vite-css-dev-test.ts @@ -335,7 +335,7 @@ test.describe("Vite CSS dev", () => { // 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 the HMR changes + // 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"