From 9d516ddd8392657d6436d2980528e1e61a8d4937 Mon Sep 17 00:00:00 2001 From: Pedro Cattori Date: Tue, 24 Oct 2023 17:00:05 -0400 Subject: [PATCH] Unstable Vite: support for custom servers (#7682) --- docs/future/vite.md | 86 ++++++- integration/helpers/dev.ts | 87 +++++++ integration/vite-dev-express-test.ts | 309 ++++++++++++++++++++++++ packages/remix-dev/index.ts | 6 +- packages/remix-dev/vite/index.ts | 16 ++ packages/remix-dev/vite/plugin.ts | 13 +- packages/remix-express/server.ts | 2 +- packages/remix-server-runtime/server.ts | 49 +++- 8 files changed, 553 insertions(+), 15 deletions(-) create mode 100644 integration/helpers/dev.ts create mode 100644 integration/vite-dev-express-test.ts diff --git a/docs/future/vite.md b/docs/future/vite.md index 6601c1dcf8c..01af7bf8d0e 100644 --- a/docs/future/vite.md +++ b/docs/future/vite.md @@ -14,7 +14,7 @@ toc: false | Feature | Node | Deno | Cloudflare | Notes | | ---------------------------- | ---- | ---- | ---------- | --------------------------------------------------------------------- | | Built-in dev server | ✅ | ❓ | ⏳ | | -| Other servers (e.g. Express) | ⏳ | ⏳ | ⏳ | | +| Other servers (e.g. Express) | ✅ | ❓ | ⏳ | | | HMR | ✅ | ❓ | ⏳ | | | HDR | ✅ | ❓ | ⏳ | | | MDX routes | ✅ | ❓ | ⏳ | [Supported with some deprecations.][supported-with-some-deprecations] | @@ -494,10 +494,92 @@ If you want to reuse values across routes, stick them in their own non-route mod export const myValue = "some value"; ``` -#### Adding and Removing Hooks +#### Changing Hooks React Fast Refresh cannot track changes for a component when hooks are being added or removed from it, causing full reloads just for the next render. After the hooks have been updated, changes should result in hot updates again. For example, if you add [`useLoaderData`][use_loader_data] to your component, you may lose state local to that component for that render. +Additionally, if you are destructuring a hook's return value, React Fast Refresh will not be able to preserve state for the component if the destructured key is removed or renamed. +For example: + +```tsx +export const loader = () => { + return json({ stuff: "some things" }); +}; + +export default function Component() { + const { stuff } = useLoaderData(); + return ( +
+ +

{stuff}

+
+ ); +} +``` + +If you change the key `stuff` to `things`: + +```diff +export const loader = () => { +- return json({ stuff: "some things" }) ++ return json({ things: "some things" }) +} + +export default Component() { +- let { stuff } = useLoaderData() ++ let { things } = useLoaderData() + return ( +
+ +-

{stuff}

++

{things}

+
+ ) +} +``` + +then React Fast Refresh will not be able to preserve state `` ❌. + +As a workaround, you could refrain from destructuring and instead use the hook's return value directly: + +```tsx +export const loader = () => { + return json({ stuff: "some things" }); +}; + +export default function Component() { + const data = useLoaderData(); + return ( +
+ +

{data.stuff}

+
+ ); +} +``` + +Now if you change the key `stuff` to `things`: + +```diff +export const loader = () => { +- return json({ things: "some things" }) ++ return json({ things: "some things" }) +} + +export default Component() { + let data = useLoaderData() + return ( +
+ +-

{data.stuff}

++

{data.things}

+
+ ) +} +``` + +then React Fast Refresh will preserve state for the ``, though you may need to use [component keys](#component-keys) as described in the next section if the stateful element (e.g. ``) is a sibling of the changed element. + #### Component Keys In some cases, React cannot distinguish between existing components being changed and new components being added. [React needs `key`s][react_keys] to disambiguate these cases and track changes when sibling elements are modified. diff --git a/integration/helpers/dev.ts b/integration/helpers/dev.ts new file mode 100644 index 00000000000..0c5af340f7b --- /dev/null +++ b/integration/helpers/dev.ts @@ -0,0 +1,87 @@ +import { spawn } from "node:child_process"; +import type { Readable } from "node:stream"; +import execa from "execa"; +import getPort from "get-port"; +import resolveBin from "resolve-bin"; +import waitOn from "wait-on"; + +const isWindows = process.platform === "win32"; + +export async function viteDev( + projectDir: string, + options: { port?: number } = {} +) { + let viteBin = resolveBin.sync("vite"); + return node(projectDir, [viteBin, "dev"], options); +} + +export async function node( + projectDir: string, + command: string[], + options: { port?: number } = {} +) { + let nodeBin = process.argv[0]; + let proc = spawn(nodeBin, command, { + cwd: projectDir, + env: process.env, + stdio: "pipe", + }); + let devStdout = bufferize(proc.stdout); + let devStderr = bufferize(proc.stderr); + + let port = options.port ?? (await getPort()); + await waitOn({ + resources: [`http://localhost:${port}/`], + timeout: 10000, + }).catch((err) => { + let stdout = devStdout(); + let stderr = devStderr(); + throw new Error( + [ + err.message, + "", + "exit code: " + proc.exitCode, + "stdout: " + stdout ? `\n${stdout}\n` : "", + "stderr: " + stderr ? `\n${stderr}\n` : "", + ].join("\n") + ); + }); + + return { pid: proc.pid!, port: port }; +} + +export async function kill(pid: number) { + if (!isAlive(pid)) return; + if (isWindows) { + await execa("taskkill", ["/F", "/PID", pid.toString()]).catch((error) => { + // taskkill 128 -> the process is already dead + if (error.exitCode === 128) return; + if (/There is no running instance of the task./.test(error.message)) + return; + console.warn(error.message); + }); + return; + } + await execa("kill", ["-9", pid.toString()]).catch((error) => { + // process is already dead + if (/No such process/.test(error.message)) return; + console.warn(error.message); + }); +} + +// utils ------------------------------------------------------------ + +function bufferize(stream: Readable): () => string { + let buffer = ""; + stream.on("data", (data) => (buffer += data.toString())); + return () => buffer; +} + +function isAlive(pid: number) { + try { + process.kill(pid, 0); + return true; + } catch (error) { + return false; + } +} diff --git a/integration/vite-dev-express-test.ts b/integration/vite-dev-express-test.ts new file mode 100644 index 00000000000..b44035766b7 --- /dev/null +++ b/integration/vite-dev-express-test.ts @@ -0,0 +1,309 @@ +import fs from "node:fs/promises"; +import path from "node:path"; +import { test, expect } from "@playwright/test"; +import getPort from "get-port"; + +import { createFixtureProject, js } from "./helpers/create-fixture.js"; +import { kill, node } from "./helpers/dev.js"; + +let projectDir: string; +let dev: { pid: number; port: number }; + +test.beforeAll(async () => { + let port = await getPort(); + let hmrPort = await getPort(); + projectDir = await createFixtureProject({ + compiler: "vite", + files: { + "vite.config.mjs": js` + import { defineConfig } from "vite"; + import { unstable_vitePlugin as remix } from "@remix-run/dev"; + + export default defineConfig({ + server: { + hmr: { + port: ${hmrPort} + } + }, + optimizeDeps: { + include: ["react", "react-dom/client"], + }, + plugins: [remix()], + }); + `, + "server.mjs": js` + import { + unstable_createViteServer, + unstable_loadViteServerBuild, + } from "@remix-run/dev"; + import { createRequestHandler } from "@remix-run/express"; + import { installGlobals } from "@remix-run/node"; + import express from "express"; + + installGlobals(); + + let vite = + process.env.NODE_ENV === "production" + ? undefined + : await unstable_createViteServer(); + + const app = express(); + + if (vite) { + app.use(vite.middlewares); + } else { + app.use( + "/build", + express.static("public/build", { immutable: true, maxAge: "1y" }) + ); + } + app.use(express.static("public", { maxAge: "1h" })); + + app.all( + "*", + createRequestHandler({ + build: vite + ? () => unstable_loadViteServerBuild(vite) + : await import("./build/index.js"), + }) + ); + + const port = ${port}; + app.listen(port, () => console.log('http://localhost:' + port)); + `, + "app/root.tsx": js` + import { Links, Meta, Outlet, Scripts, LiveReload } from "@remix-run/react"; + + export default function Root() { + return ( + + + + + + +
+

Root

+ +
+ + + + + ); + } + `, + "app/routes/_index.tsx": js` + // imports + import { useState, useEffect } from "react"; + + // loader + + export default function IndexRoute() { + // hooks + const [mounted, setMounted] = useState(false); + useEffect(() => { + setMounted(true); + }, []); + + return ( +
+

Index

+ +

Mounted: {mounted ? "yes" : "no"}

+

HMR updated: 0

+ {/* elements */} +
+ ); + } + `, + }, + }); + dev = await node(projectDir, ["./server.mjs"], { port }); +}); + +test.afterAll(async () => { + await kill(dev.pid); +}); + +test("Vite custom server HMR & HDR", async ({ page }) => { + // setup: initial render + await page.goto(`http://localhost:${dev.port}/`, { + waitUntil: "networkidle", + }); + await expect(page.locator("#index [data-title]")).toHaveText("Index"); + + // setup: hydration + await expect(page.locator("#index [data-mounted]")).toHaveText( + "Mounted: yes" + ); + + // setup: browser state + let hmrStatus = page.locator("#index [data-hmr]"); + await expect(hmrStatus).toHaveText("HMR updated: 0"); + let input = page.locator("#index input"); + await expect(input).toBeVisible(); + await input.type("stateful"); + + // route: HMR + await edit("app/routes/_index.tsx", (contents) => + contents.replace("HMR updated: 0", "HMR updated: 1") + ); + await page.waitForLoadState("networkidle"); + await expect(hmrStatus).toHaveText("HMR updated: 1"); + await expect(input).toHaveValue("stateful"); + + // route: add loader + await edit("app/routes/_index.tsx", (contents) => + contents + .replace( + "// imports", + `// imports\nimport { json } from "@remix-run/node";\nimport { useLoaderData } from "@remix-run/react"` + ) + .replace( + "// loader", + `// loader\nexport const loader = () => json({ message: "HDR updated: 0" });` + ) + .replace( + "// hooks", + "// hooks\nconst { message } = useLoaderData();" + ) + .replace( + "{/* elements */}", + `{/* elements */}\n

{message}

` + ) + ); + await page.waitForLoadState("networkidle"); + let hdrStatus = page.locator("#index [data-hdr]"); + await expect(hdrStatus).toHaveText("HDR updated: 0"); + // React Fast Refresh cannot preserve state for a component when hooks are added or removed + await expect(input).toHaveValue(""); + await input.type("stateful"); + + // route: HDR + await edit("app/routes/_index.tsx", (contents) => + contents.replace("HDR updated: 0", "HDR updated: 1") + ); + await page.waitForLoadState("networkidle"); + await expect(hdrStatus).toHaveText("HDR updated: 1"); + await expect(input).toHaveValue("stateful"); + + // route: HMR + HDR + await edit("app/routes/_index.tsx", (contents) => + contents + .replace("HMR updated: 1", "HMR updated: 2") + .replace("HDR updated: 1", "HDR updated: 2") + ); + await page.waitForLoadState("networkidle"); + await expect(hmrStatus).toHaveText("HMR updated: 2"); + await expect(hdrStatus).toHaveText("HDR updated: 2"); + await expect(input).toHaveValue("stateful"); + + // create new non-route component module + await fs.writeFile( + path.join(projectDir, "app/component.tsx"), + js` + export function MyComponent() { + return

Component HMR: 0

; + } + `, + "utf8" + ); + await edit("app/routes/_index.tsx", (contents) => + contents + .replace( + "// imports", + `// imports\nimport { MyComponent } from "../component";` + ) + .replace("{/* elements */}", "{/* elements */}\n") + ); + await page.waitForLoadState("networkidle"); + let component = page.locator("#index [data-component]"); + await expect(component).toBeVisible(); + await expect(component).toHaveText("Component HMR: 0"); + await expect(input).toHaveValue("stateful"); + + // non-route: HMR + await edit("app/component.tsx", (contents) => + contents.replace("Component HMR: 0", "Component HMR: 1") + ); + await page.waitForLoadState("networkidle"); + await expect(component).toHaveText("Component HMR: 1"); + await expect(input).toHaveValue("stateful"); + + // create new non-route server module + await fs.writeFile( + path.join(projectDir, "app/indirect-hdr-dep.ts"), + js`export const indirect = "indirect 0"`, + "utf8" + ); + await fs.writeFile( + path.join(projectDir, "app/direct-hdr-dep.ts"), + js` + import { indirect } from "./indirect-hdr-dep" + export const direct = "direct 0 & " + indirect + `, + "utf8" + ); + await edit("app/routes/_index.tsx", (contents) => + contents + .replace( + "// imports", + `// imports\nimport { direct } from "../direct-hdr-dep"` + ) + .replace( + `json({ message: "HDR updated: 2" })`, + `json({ message: "HDR updated: " + direct })` + ) + ); + await page.waitForLoadState("networkidle"); + await expect(hdrStatus).toHaveText("HDR updated: direct 0 & indirect 0"); + await expect(input).toHaveValue("stateful"); + + // non-route: HDR for direct dependency + await edit("app/direct-hdr-dep.ts", (contents) => + contents.replace("direct 0 &", "direct 1 &") + ); + await page.waitForLoadState("networkidle"); + await expect(hdrStatus).toHaveText("HDR updated: direct 1 & indirect 0"); + await expect(input).toHaveValue("stateful"); + + // non-route: HDR for indirect dependency + await edit("app/indirect-hdr-dep.ts", (contents) => + contents.replace("indirect 0", "indirect 1") + ); + await page.waitForLoadState("networkidle"); + await expect(hdrStatus).toHaveText("HDR updated: direct 1 & indirect 1"); + await expect(input).toHaveValue("stateful"); + + // everything everywhere all at once + await Promise.all([ + edit("app/routes/_index.tsx", (contents) => + contents + .replace("HMR updated: 2", "HMR updated: 3") + .replace("HDR updated: ", "HDR updated: route & ") + ), + edit("app/component.tsx", (contents) => + contents.replace("Component HMR: 1", "Component HMR: 2") + ), + edit("app/direct-hdr-dep.ts", (contents) => + contents.replace("direct 1 &", "direct 2 &") + ), + edit("app/indirect-hdr-dep.ts", (contents) => + contents.replace("indirect 1", "indirect 2") + ), + ]); + await page.waitForLoadState("networkidle"); + await expect(hmrStatus).toHaveText("HMR updated: 3"); + await expect(component).toHaveText("Component HMR: 2"); + await expect(hdrStatus).toHaveText( + "HDR updated: route & direct 2 & indirect 2" + ); + await expect(input).toHaveValue("stateful"); +}); + +async function edit(file: string, transform: (contents: string) => string) { + let filepath = path.join(projectDir, file); + let contents = await fs.readFile(filepath, "utf8"); + await fs.writeFile(filepath, transform(contents), "utf8"); +} diff --git a/packages/remix-dev/index.ts b/packages/remix-dev/index.ts index 4dbdb3faf21..2c8f8eca5da 100644 --- a/packages/remix-dev/index.ts +++ b/packages/remix-dev/index.ts @@ -6,4 +6,8 @@ export * as cli from "./cli/index"; export type { Manifest as AssetsManifest } from "./manifest"; export { getDependenciesToBundle } from "./dependencies"; -export { unstable_vitePlugin } from "./vite"; +export { + unstable_vitePlugin, + unstable_createViteServer, + unstable_loadViteServerBuild, +} from "./vite"; diff --git a/packages/remix-dev/vite/index.ts b/packages/remix-dev/vite/index.ts index 1bcb08bb692..70dcf55c5d4 100644 --- a/packages/remix-dev/vite/index.ts +++ b/packages/remix-dev/vite/index.ts @@ -1,10 +1,26 @@ // This file allows us to dynamically require the plugin so non-Vite consumers // don't need to have Vite installed as a peer dependency. Only types should // be imported at the top level. +import type { ViteDevServer } from "vite"; + import type { RemixVitePlugin } from "./plugin"; +import { id } from "./vmod"; export const unstable_vitePlugin: RemixVitePlugin = (...args) => { // eslint-disable-next-line @typescript-eslint/consistent-type-imports let { remixVitePlugin } = require("./plugin") as typeof import("./plugin"); return remixVitePlugin(...args); }; + +export const unstable_createViteServer = async () => { + let vite = await import("vite"); + return vite.createServer({ + server: { + middlewareMode: true, + }, + }); +}; + +export const unstable_loadViteServerBuild = async (vite: ViteDevServer) => { + return vite.ssrLoadModule(id("server-entry")); +}; diff --git a/packages/remix-dev/vite/plugin.ts b/packages/remix-dev/vite/plugin.ts index 602ad8ae039..44963c33dfb 100644 --- a/packages/remix-dev/vite/plugin.ts +++ b/packages/remix-dev/vite/plugin.ts @@ -461,6 +461,13 @@ export const remixVitePlugin: RemixVitePlugin = (options = {}) => { viteChildCompiler = await createViteDevServer({ ...viteUserConfig, + server: { + ...viteUserConfig.server, + // when parent compiler runs in middleware mode to support + // custom servers, we don't want the child compiler also + // run in middleware mode as that will cause websocket port conflicts + middlewareMode: false, + }, configFile: false, envFile: false, plugins: [ @@ -852,8 +859,10 @@ function getRoute( pluginConfig: ResolvedRemixVitePluginConfig, file: string ): Route | undefined { - if (!file.startsWith(pluginConfig.appDirectory)) return; - let routePath = path.relative(pluginConfig.appDirectory, file); + if (!file.startsWith(viteNormalizePath(pluginConfig.appDirectory))) return; + let routePath = viteNormalizePath( + path.relative(pluginConfig.appDirectory, file) + ); let route = Object.values(pluginConfig.routes).find( (r) => r.file === routePath ); diff --git a/packages/remix-express/server.ts b/packages/remix-express/server.ts index 18bede9a73a..016cdfb1d9b 100644 --- a/packages/remix-express/server.ts +++ b/packages/remix-express/server.ts @@ -37,7 +37,7 @@ export function createRequestHandler({ getLoadContext, mode = process.env.NODE_ENV, }: { - build: ServerBuild; + build: ServerBuild | (() => Promise); getLoadContext?: GetLoadContextFunction; mode?: string; }): RequestHandler { diff --git a/packages/remix-server-runtime/server.ts b/packages/remix-server-runtime/server.ts index c405a2fc4d2..c49b28b721e 100644 --- a/packages/remix-server-runtime/server.ts +++ b/packages/remix-server-runtime/server.ts @@ -12,7 +12,7 @@ import { } from "@remix-run/router"; import type { AppLoadContext } from "./data"; -import type { ServerBuild } from "./build"; +import type { HandleErrorFunction, ServerBuild } from "./build"; import type { EntryContext } from "./entry"; import { createEntryRouteModules } from "./entry"; import { sanitizeErrors, serializeError, serializeErrors } from "./errors"; @@ -20,6 +20,7 @@ import { getDocumentHeadersRR } from "./headers"; import invariant from "./invariant"; import { ServerMode, isServerMode } from "./mode"; import { matchServerRoutes } from "./routeMatching"; +import type { ServerRoute } from "./routes"; import { createStaticHandlerDataRoutes, createRoutes } from "./routes"; import { createDeferredReadableStream, @@ -35,14 +36,11 @@ export type RequestHandler = ( ) => Promise; export type CreateRequestHandlerFunction = ( - build: ServerBuild, + build: ServerBuild | (() => Promise), mode?: string ) => RequestHandler; -export const createRequestHandler: CreateRequestHandlerFunction = ( - build, - mode -) => { +function derive(build: ServerBuild, mode?: string) { let routes = createRoutes(build.routes); let dataRoutes = createStaticHandlerDataRoutes(build.routes, build.future); let serverMode = isServerMode(mode) ? mode : ServerMode.Production; @@ -58,12 +56,45 @@ export const createRequestHandler: CreateRequestHandlerFunction = ( ); } }); + return { + routes, + dataRoutes, + serverMode, + staticHandler, + errorHandler, + }; +} + +export const createRequestHandler: CreateRequestHandlerFunction = ( + build, + mode +) => { + let _build: ServerBuild; + let routes: ServerRoute[]; + let serverMode: ServerMode; + let staticHandler: StaticHandler; + let errorHandler: HandleErrorFunction; return async function requestHandler( request, loadContext = {}, { criticalCss } = {} ) { + _build = typeof build === "function" ? await build() : build; + if (typeof build === "function") { + let derived = derive(_build, mode); + routes = derived.routes; + serverMode = derived.serverMode; + staticHandler = derived.staticHandler; + errorHandler = derived.errorHandler; + } else if (!routes || !serverMode || !staticHandler || !errorHandler) { + let derived = derive(_build, mode); + routes = derived.routes; + serverMode = derived.serverMode; + staticHandler = derived.staticHandler; + errorHandler = derived.errorHandler; + } + let url = new URL(request.url); let matches = matchServerRoutes(routes, url.pathname); @@ -87,8 +118,8 @@ export const createRequestHandler: CreateRequestHandlerFunction = ( handleError ); - if (build.entry.module.handleDataRequest) { - response = await build.entry.module.handleDataRequest(response, { + if (_build.entry.module.handleDataRequest) { + response = await _build.entry.module.handleDataRequest(response, { context: loadContext, params: matches?.find((m) => m.route.id == routeId)?.params || {}, request, @@ -110,7 +141,7 @@ export const createRequestHandler: CreateRequestHandlerFunction = ( } else { response = await handleDocumentRequestRR( serverMode, - build, + _build, staticHandler, request, loadContext,