fix(dev): gracefully handle hdr errors (#6467)

pcattori · web-flow · commit 9efdaea44893 · 2023-05-24T11:58:26.000-04:00
diff --git a/.changeset/strong-dolphins-tell.md b/.changeset/strong-dolphins-tell.md
@@ -0,0 +1,5 @@
+---
+"@remix-run/dev": patch
+---
+
+fix dev server crashes caused by ungraceful hdr error handling
diff --git a/integration/hmr-log-test.ts b/integration/hmr-log-test.ts
@@ -463,6 +463,49 @@ whatsup
     fs.writeFileSync(mdxPath, originalMdx);
     await page.waitForSelector(`#crazy`);
     expect(dataRequests).toBe(5);
+
+    // dev server doesn't crash when rebuild fails
+    await page.click(`a[href="/"]`);
+    await page.getByText("Hello, planet").waitFor({ timeout: HMR_TIMEOUT_MS });
+    await page.waitForLoadState("networkidle");
+
+    expect(devStderr()).toBe("");
+    let withSyntaxError = `
+      import { useLoaderData } from "@remix-run/react";
+      export function shouldRevalidate(args) {
+        return true;
+      }
+      eport efault functio Index() {
+        const t = useLoaderData();
+        return (
+          <mai>
+            <h1>With Syntax Error</h1>
+          </main>
+        )
+      }
+    `;
+    fs.writeFileSync(indexPath, withSyntaxError);
+    await wait(() => devStderr().includes('Expected ";" but found "efault"'), {
+      timeoutMs: HMR_TIMEOUT_MS,
+    });
+
+    let withFix = `
+      import { useLoaderData } from "@remix-run/react";
+      export function shouldRevalidate(args) {
+        return true;
+      }
+      export default function Index() {
+        // const t = useLoaderData();
+        return (
+          <main>
+            <h1>With Fix</h1>
+          </main>
+        )
+      }
+    `;
+    fs.writeFileSync(indexPath, withFix);
+    await page.waitForLoadState("networkidle");
+    await page.getByText("With Fix").waitFor({ timeout: HMR_TIMEOUT_MS });
   } catch (e) {
     console.log("stdout begin -----------------------");
     console.log(devStdout());
diff --git a/integration/hmr-test.ts b/integration/hmr-test.ts
@@ -462,6 +462,49 @@ whatsup
     fs.writeFileSync(mdxPath, originalMdx);
     await page.waitForSelector(`#crazy`);
     expect(dataRequests).toBe(5);
+
+    // dev server doesn't crash when rebuild fails
+    await page.click(`a[href="/"]`);
+    await page.getByText("Hello, planet").waitFor({ timeout: HMR_TIMEOUT_MS });
+    await page.waitForLoadState("networkidle");
+
+    expect(devStderr()).toBe("");
+    let withSyntaxError = `
+      import { useLoaderData } from "@remix-run/react";
+      export function shouldRevalidate(args) {
+        return true;
+      }
+      eport efault functio Index() {
+        const t = useLoaderData();
+        return (
+          <mai>
+            <h1>With Syntax Error</h1>
+          </main>
+        )
+      }
+    `;
+    fs.writeFileSync(indexPath, withSyntaxError);
+    await wait(() => devStderr().includes('Expected ";" but found "efault"'), {
+      timeoutMs: HMR_TIMEOUT_MS,
+    });
+
+    let withFix = `
+      import { useLoaderData } from "@remix-run/react";
+      export function shouldRevalidate(args) {
+        return true;
+      }
+      export default function Index() {
+        // const t = useLoaderData();
+        return (
+          <main>
+            <h1>With Fix</h1>
+          </main>
+        )
+      }
+    `;
+    fs.writeFileSync(indexPath, withFix);
+    await page.waitForLoadState("networkidle");
+    await page.getByText("With Fix").waitFor({ timeout: HMR_TIMEOUT_MS });
   } catch (e) {
     console.log("stdout begin -----------------------");
     console.log(devStdout());
diff --git a/packages/remix-dev/devServer_unstable/hdr.ts b/packages/remix-dev/devServer_unstable/hdr.ts
@@ -16,7 +16,9 @@ function isBareModuleId(id: string): boolean {
 
 type Route = Context["config"]["routes"][string];
 
-export let detectLoaderChanges = async (ctx: Context) => {
+export let detectLoaderChanges = async (
+  ctx: Context
+): Promise<Record<string, string>> => {
   let entryPoints: Record<string, string> = {};
   for (let id of Object.keys(ctx.config.routes)) {
     entryPoints[id] = ctx.config.routes[id].file + "?loader";
@@ -30,6 +32,7 @@ export let detectLoaderChanges = async (ctx: Context) => {
     write: false,
     entryNames: "[hash]",
     loader: loaders,
+    logLevel: "silent",
     plugins: [
       {
         name: "hmr-loader",
@@ -98,13 +101,13 @@ export let detectLoaderChanges = async (ctx: Context) => {
   };
 
   let { metafile } = await esbuild.build(options);
-  let entries = Object.entries(metafile!.outputs).map(
-    ([hashjs, { entryPoint }]) => {
-      let file = entryPoint
-        ?.replace(/^hmr-loader:/, "")
-        ?.replace(/\?loader$/, "");
-      return [file, hashjs.replace(/\.js$/, "")];
-    }
-  );
-  return Object.fromEntries(entries);
+
+  let entries: Record<string, string> = {};
+  for (let [hashjs, { entryPoint }] of Object.entries(metafile!.outputs)) {
+    if (entryPoint === undefined) continue;
+    let file = entryPoint.replace(/^hmr-loader:/, "").replace(/\?loader$/, "");
+    entries[file] = hashjs.replace(/\.js$/, "");
+  }
+
+  return entries;
 };
diff --git a/packages/remix-dev/devServer_unstable/index.ts b/packages/remix-dev/devServer_unstable/index.ts
@@ -15,6 +15,8 @@ import * as HMR from "./hmr";
 import { warnOnce } from "../warnOnce";
 import { detectPackageManager } from "../cli/detectPackageManager";
 import * as HDR from "./hdr";
+import type { Result } from "../result";
+import { err, ok } from "../result";
 
 type Origin = {
   scheme: string;
@@ -59,7 +61,7 @@ export let serve = async (
     manifest?: Manifest;
     prevManifest?: Manifest;
     appReady?: Channel.Type<void>;
-    hdr?: Promise<Record<string, string>>;
+    loaderChanges?: Promise<Result<Record<string, string>>>;
     prevLoaderHashes?: Record<string, string>;
   } = {};
 
@@ -122,57 +124,70 @@ export let serve = async (
     },
     {
       onBuildStart: async (ctx) => {
+        // stop listening for previous manifest
         state.appReady?.err();
+
         clean(ctx.config);
         websocket.log(state.prevManifest ? "Rebuilding..." : "Building...");
 
-        state.hdr = HDR.detectLoaderChanges(ctx);
+        state.loaderChanges = HDR.detectLoaderChanges(ctx).then(ok, err);
       },
       onBuildManifest: (manifest: Manifest) => {
         state.manifest = manifest;
+        state.appReady = Channel.create();
       },
       onBuildFinish: async (ctx, durationMs, succeeded) => {
         if (!succeeded) return;
-
         websocket.log(
           (state.prevManifest ? "Rebuilt" : "Built") +
             ` in ${prettyMs(durationMs)}`
         );
-        state.appReady = Channel.create();
 
-        let start = Date.now();
-        console.log(`Waiting for app server (${state.manifest?.version})`);
-        if (
-          options.command &&
-          (state.appServer === undefined || options.restart)
-        ) {
-          await kill(state.appServer);
-          state.appServer = startAppServer(options.command);
-        }
-        let { ok } = await state.appReady.result;
-        // result not ok -> new build started before this one finished. do not process outdated manifest
-        let loaderHashes = await state.hdr;
-        if (ok) {
+        // accumulate new state, but only update state after updates are processed
+        let newState: typeof state = { prevManifest: state.manifest };
+        try {
+          console.log(`Waiting for app server (${state.manifest?.version})`);
+          let start = Date.now();
+          if (
+            options.command &&
+            (state.appServer === undefined || options.restart)
+          ) {
+            await kill(state.appServer);
+            state.appServer = startAppServer(options.command);
+          }
+          let appReady = await state.appReady!.result;
+          if (!appReady.ok) return;
           console.log(`App server took ${prettyMs(Date.now() - start)}`);
-          if (state.manifest && loaderHashes && state.prevManifest) {
+
+          // HMR + HDR
+          let loaderChanges = await state.loaderChanges!;
+          if (loaderChanges.ok) {
+            newState.prevLoaderHashes = loaderChanges.value;
+          }
+          if (loaderChanges?.ok && state.manifest && state.prevManifest) {
             let updates = HMR.updates(
               ctx.config,
               state.manifest,
               state.prevManifest,
-              loaderHashes,
+              loaderChanges.value,
               state.prevLoaderHashes
             );
             websocket.hmr(state.manifest, updates);
 
             let hdr = updates.some((u) => u.revalidate);
             console.log("> HMR" + (hdr ? " + HDR" : ""));
-          } else if (state.prevManifest !== undefined) {
+            return;
+          }
+
+          // Live Reload
+          if (state.prevManifest !== undefined) {
             websocket.reload();
             console.log("> Live reload");
           }
+        } finally {
+          // commit accumulated state
+          Object.assign(state, newState);
         }
-        state.prevManifest = state.manifest;
-        state.prevLoaderHashes = loaderHashes;
       },
       onFileCreated: (file) =>
         websocket.log(`File created: ${relativePath(file)}`),
