feat(dev): cross-module loader change detection (#6299)

remix-run · May 5, 2023 · f5ef1ac · f5ef1ac
1 parent 0b6bc2c
commit f5ef1ac
Show file tree

Hide file tree

Showing 21 changed files with 261 additions and 86 deletions.
diff --git a/.changeset/afraid-coats-count.md b/.changeset/afraid-coats-count.md
@@ -0,0 +1,6 @@
+---
+"@remix-run/dev": patch
+"@remix-run/react": patch
+---
+
+cross-module loader change detection for hdr
diff --git a/integration/hmr-log-test.ts b/integration/hmr-log-test.ts
@@ -125,6 +125,11 @@ let fixture = (options: {
         ...cssBundleHref ? [{ rel: "stylesheet", href: cssBundleHref }] : [],
       ];
 
+      // dummy loader to make sure that HDR is granular
+      export const loader = () => {
+        return null;
+      };
+
       export default function Root() {
         return (
           <html lang="en" className="h-full">
@@ -141,6 +146,7 @@ let fixture = (options: {
                   <ul>
                     <li><Link to="/">Home</Link></li>
                     <li><Link to="/about">About</Link></li>
+                    <li><Link to="/mdx">MDX</Link></li>
                   </ul>
                 </nav>
               </header>
@@ -179,6 +185,18 @@ let fixture = (options: {
         )
       }
     `,
+    "app/routes/mdx.mdx": `import { useLoaderData } from '@remix-run/react'
+export const loader = () => "crazy"
+export const Component = () => {
+  const data = useLoaderData()
+  return <h1 id={data}>{data}</h1>
+}
+
+# heyo
+whatsup
+
+<Component/>
+`,
 
     "app/components/counter.tsx": js`
       import * as React from "react";
@@ -272,6 +290,8 @@ test("HMR", async ({ page }) => {
     let originalCounter = fs.readFileSync(counterPath, "utf8");
     let cssModulePath = path.join(projectDir, "app", "styles.module.css");
     let originalCssModule = fs.readFileSync(cssModulePath, "utf8");
+    let mdxPath = path.join(projectDir, "app", "routes", "mdx.mdx");
+    let originalMdx = fs.readFileSync(mdxPath, "utf8");
 
     // make content and style changed to index route
     let newCssModule = `
@@ -419,9 +439,30 @@ test("HMR", async ({ page }) => {
       `#about-counter:has-text("inc 0")`
     );
 
-    // This should not have triggered any revalidation but our detection is
-    // failing for x-module changes for route module imports
-    // expect(dataRequests).toBe(2);
+    expect(dataRequests).toBe(2);
+
+    // mdx
+    await page.click(`a[href="/mdx"]`);
+    await page.waitForSelector(`#crazy`);
+    let mdx = `import { useLoaderData } from '@remix-run/react'
+export const loader = () => "hot"
+export const Component = () => {
+  const data = useLoaderData()
+  return <h1 id={data}>{data}</h1>
+}
+
+# heyo
+whatsup
+
+<Component/>
+`;
+    fs.writeFileSync(mdxPath, mdx);
+    await page.waitForSelector(`#hot`);
+    expect(dataRequests).toBe(4);
+
+    fs.writeFileSync(mdxPath, originalMdx);
+    await page.waitForSelector(`#crazy`);
+    expect(dataRequests).toBe(5);
   } catch (e) {
     console.log("stdout begin -----------------------");
     console.log(devStdout());

diff --git a/integration/hmr-test.ts b/integration/hmr-test.ts
@@ -125,6 +125,11 @@ let fixture = (options: {
         ...cssBundleHref ? [{ rel: "stylesheet", href: cssBundleHref }] : [],
       ];
 
+      // dummy loader to make sure that HDR is granular
+      export const loader = () => {
+        return null;
+      };
+
       export default function Root() {
         return (
           <html lang="en" className="h-full">
@@ -141,6 +146,7 @@ let fixture = (options: {
                   <ul>
                     <li><Link to="/">Home</Link></li>
                     <li><Link to="/about">About</Link></li>
+                    <li><Link to="/mdx">MDX</Link></li>
                   </ul>
                 </nav>
               </header>
@@ -179,7 +185,18 @@ let fixture = (options: {
         )
       }
     `,
-
+    "app/routes/mdx.mdx": `import { useLoaderData } from '@remix-run/react'
+export const loader = () => "crazy"
+export const Component = () => {
+  const data = useLoaderData()
+  return <h1 id={data}>{data}</h1>
+}
+
+# heyo
+whatsup
+
+<Component/>
+`,
     "app/components/counter.tsx": js`
       import * as React from "react";
       export default function Counter({ id }) {
@@ -272,6 +289,8 @@ test("HMR", async ({ page }) => {
     let originalCounter = fs.readFileSync(counterPath, "utf8");
     let cssModulePath = path.join(projectDir, "app", "styles.module.css");
     let originalCssModule = fs.readFileSync(cssModulePath, "utf8");
+    let mdxPath = path.join(projectDir, "app", "routes", "mdx.mdx");
+    let originalMdx = fs.readFileSync(mdxPath, "utf8");
 
     // make content and style changed to index route
     let newCssModule = `
@@ -419,9 +438,30 @@ test("HMR", async ({ page }) => {
       `#about-counter:has-text("inc 0")`
     );
 
-    // This should not have triggered any revalidation but our detection is
-    // failing for x-module changes for route module imports
-    // expect(dataRequests).toBe(2);
+    expect(dataRequests).toBe(2);
+
+    // mdx
+    await page.click(`a[href="/mdx"]`);
+    await page.waitForSelector(`#crazy`);
+    let mdx = `import { useLoaderData } from '@remix-run/react'
+export const loader = () => "hot"
+export const Component = () => {
+  const data = useLoaderData()
+  return <h1 id={data}>{data}</h1>
+}
+
+# heyo
+whatsup
+
+<Component/>
+`;
+    fs.writeFileSync(mdxPath, mdx);
+    await page.waitForSelector(`#hot`);
+    expect(dataRequests).toBe(4);
+
+    fs.writeFileSync(mdxPath, originalMdx);
+    await page.waitForSelector(`#crazy`);
+    expect(dataRequests).toBe(5);
   } catch (e) {
     console.log("stdout begin -----------------------");
     console.log(devStdout());

diff --git a/packages/remix-dev/__tests__/cli-test.ts b/packages/remix-dev/__tests__/cli-test.ts
@@ -122,7 +122,7 @@ describe("remix CLI", () => {
             --http-host         HTTP(S) host for the dev server. Default: localhost
             --http-port         HTTP(S) port for the dev server. Default: any open port
             --no-restart        Do not restart the app server when rebuilds occur.
-            --websocket-port    Websocket port for the dev server. Default: any open port
+            --websocket-port    WebSocket port for the dev server. Default: any open port
           \`init\` Options:
             --no-delete         Skip deleting the \`remix.init\` script
           \`routes\` Options:

diff --git a/packages/remix-dev/cli/commands.ts b/packages/remix-dev/cli/commands.ts
@@ -183,7 +183,7 @@ export async function build(
       host: dev.httpHost,
       port: dev.httpPort,
     };
-    options.devWebsocketPort = dev.websocketPort;
+    options.devWebSocketPort = dev.webSocketPort;
   }
 
   fse.emptyDirSync(config.assetsBuildDirectory);
@@ -475,7 +475,7 @@ type DevBuildFlags = {
   httpScheme: string;
   httpHost: string;
   httpPort: number;
-  websocketPort: number;
+  webSocketPort: number;
 };
 let resolveDevBuild = async (
   config: RemixConfig,
@@ -500,16 +500,16 @@ let resolveDevBuild = async (
     (dev === true ? undefined : dev.httpPort) ??
     (await findPort());
   // prettier-ignore
-  let websocketPort =
-    flags.websocketPort ??
-    (dev === true ? undefined : dev.websocketPort) ??
+  let webSocketPort =
+    flags.webSocketPort ??
+    (dev === true ? undefined : dev.webSocketPort) ??
     (await findPort());
 
   return {
     httpScheme,
     httpHost,
     httpPort,
-    websocketPort,
+    webSocketPort,
   };
 };
 
@@ -524,7 +524,7 @@ let resolveDevServe = async (
   let dev = config.future.unstable_dev;
   if (dev === false) throw Error("Cannot resolve dev options");
 
-  let { httpScheme, httpHost, httpPort, websocketPort } = await resolveDevBuild(
+  let { httpScheme, httpHost, httpPort, webSocketPort } = await resolveDevBuild(
     config,
     flags
   );
@@ -561,7 +561,7 @@ let resolveDevServe = async (
     httpScheme,
     httpHost,
     httpPort,
-    websocketPort,
+    webSocketPort,
     restart,
   };
 };
diff --git a/packages/remix-dev/cli/run.ts b/packages/remix-dev/cli/run.ts
@@ -48,7 +48,7 @@ ${colors.logoBlue("R")} ${colors.logoGreen("E")} ${colors.logoYellow(
     --http-host         HTTP(S) host for the dev server. Default: localhost
     --http-port         HTTP(S) port for the dev server. Default: any open port
     --no-restart        Do not restart the app server when rebuilds occur.
-    --websocket-port    Websocket port for the dev server. Default: any open port
+    --websocket-port    WebSocket port for the dev server. Default: any open port
   \`init\` Options:
     --no-delete         Skip deleting the \`remix.init\` script
   \`routes\` Options:
@@ -226,7 +226,7 @@ export async function run(argv: string[] = process.argv.slice(2)) {
     delete flags["http-port"];
   }
   if (flags["websocket-port"]) {
-    flags.websocketPort = flags["websocket-port"];
+    flags.webSocketPort = flags["websocket-port"];
     delete flags["websocket-port"];
   }
 

diff --git a/packages/remix-dev/compiler/compiler.ts b/packages/remix-dev/compiler/compiler.ts
@@ -93,7 +93,6 @@ export let create = async (ctx: Context): Promise<Compiler> => {
     }
 
     // js compilation (implicitly writes artifacts/js)
-    // TODO: js task should not return metafile, but rather js assets
     let js = await tasks.js;
     if (!js.ok) throw error ?? js.error;
     let { metafile, hmr } = js.value;

diff --git a/packages/remix-dev/compiler/js/compiler.ts b/packages/remix-dev/compiler/js/compiler.ts
@@ -72,7 +72,6 @@ const getExternals = (remixConfig: RemixConfig): string[] => {
 
 const createEsbuildConfig = (
   ctx: Context,
-  onLoader: (filename: string, code: string) => void,
   channels: { cssBundleHref: Channel.Type<string | undefined> }
 ): esbuild.BuildOptions => {
   let entryPoints: Record<string, string> = {
@@ -142,7 +141,7 @@ const createEsbuildConfig = (
     absoluteCssUrlsPlugin(),
     externalPlugin(/^https?:\/\//, { sideEffects: false }),
     ctx.config.future.unstable_dev
-      ? browserRouteModulesPlugin_v2(ctx, routeModulePaths, onLoader)
+      ? browserRouteModulesPlugin_v2(ctx, routeModulePaths)
       : browserRouteModulesPlugin(ctx, /\?browser$/),
     mdxPlugin(ctx),
     emptyModulesPlugin(ctx, /\.server(\.[jt]sx?)?$/),
@@ -237,19 +236,12 @@ export const create = async (
   ctx: Context,
   channels: { cssBundleHref: Channel.Type<string | undefined> }
 ): Promise<Compiler> => {
-  let hmrRoutes: Record<string, { loaderHash: string }> = {};
-  let onLoader = (filename: string, code: string) => {
-    let key = path.relative(ctx.config.rootDirectory, filename);
-    hmrRoutes[key] = { loaderHash: code };
-  };
-
   let compiler = await esbuild.context({
-    ...createEsbuildConfig(ctx, onLoader, channels),
+    ...createEsbuildConfig(ctx, channels),
     metafile: true,
   });
 
   let compile = async () => {
-    hmrRoutes = {};
     let { metafile } = await compiler.rebuild();
 
     let hmr: Manifest["hmr"] | undefined = undefined;
@@ -266,7 +258,6 @@ export const create = async (
         );
       hmr = {
         runtime: hmrRuntime,
-        routes: hmrRoutes,
         timestamp: Date.now(),
       };
     }

diff --git a/packages/remix-dev/compiler/js/plugins/routes_unstable.ts b/packages/remix-dev/compiler/js/plugins/routes_unstable.ts
@@ -12,7 +12,7 @@ import { processMDX } from "../../plugins/mdx";
 
 const serverOnlyExports = new Set(["action", "loader"]);
 
-let removeServerExports = (onLoader: (loader: string) => void) =>
+let removeServerExports = () =>
   Transform.create(({ types: t }) => {
     return {
       visitor: {
@@ -38,20 +38,12 @@ let removeServerExports = (onLoader: (loader: string) => void) =>
           if (t.isFunctionDeclaration(node.declaration)) {
             let name = node.declaration.id?.name;
             if (!name) return;
-            if (name === "loader") {
-              let { code } = generate(node);
-              onLoader(code);
-            }
             if (serverOnlyExports.has(name)) return path.remove();
           }
           if (t.isVariableDeclaration(node.declaration)) {
             let declarations = node.declaration.declarations.filter((d) => {
               let name = t.isIdentifier(d.id) ? d.id.name : undefined;
               if (!name) return false;
-              if (name === "loader") {
-                let { code } = generate(node);
-                onLoader(code);
-              }
               return !serverOnlyExports.has(name);
             });
             if (declarations.length === 0) return path.remove();
@@ -72,8 +64,7 @@ let removeServerExports = (onLoader: (loader: string) => void) =>
  */
 export function browserRouteModulesPlugin(
   { config, options }: Context,
-  routeModulePaths: Map<string, string>,
-  onLoader: (filename: string, code: string) => void
+  routeModulePaths: Map<string, string>
 ): esbuild.Plugin {
   return {
     name: "browser-route-modules",
@@ -119,9 +110,7 @@ export function browserRouteModulesPlugin(
               return mdxResult;
             }
 
-            let transform = removeServerExports((loader: string) =>
-              onLoader(routeFile, loader)
-            );
+            let transform = removeServerExports();
             mdxResult.contents = transform(
               mdxResult.contents,
               // Trick babel into allowing JSX syntax.
@@ -134,10 +123,7 @@ export function browserRouteModulesPlugin(
 
           let value = cache.get(file);
           if (!value || value.sourceCode !== sourceCode) {
-            let extractedLoader;
-            let transform = removeServerExports(
-              (loader: string) => (extractedLoader = loader)
-            );
+            let transform = removeServerExports();
             let contents = transform(sourceCode, routeFile);
 
             if (options.mode === "development" && config.future.unstable_dev) {
@@ -153,7 +139,6 @@ export function browserRouteModulesPlugin(
             }
             value = {
               sourceCode,
-              extractedLoader,
               output: {
                 contents,
                 loader: getLoaderForFile(routeFile),
@@ -163,10 +148,6 @@ export function browserRouteModulesPlugin(
             cache.set(file, value);
           }
 
-          if (value.extractedLoader) {
-            onLoader(routeFile, value.extractedLoader);
-          }
-
           return value.output;
         }
       );