Throw module server errors from runJS (#190)

Co-authored-by: Gerardo Rodriguez <gerardo@cloudfour.com>

Author: calebeby

.changeset/funny-ghosts-dance.md

@@ -0,0 +1,5 @@
+---
+'pleasantest': minor
+---
+
+Improve error message output for resolution errors and syntax errors/transform errors

src/index.ts

@@ -20,6 +20,7 @@ import type { ModuleServerOpts } from './module-server';
 import { createModuleServer } from './module-server';
 import { cleanupClientRuntimeServer } from './module-server/client-runtime-server';
 import { Console } from 'console';
+import { createBuildStatusTracker } from './module-server/build-status-tracker';
 
 export { JSHandle, ElementHandle } from 'puppeteer';
 koloristOpts.enabled = true;
@@ -265,6 +266,16 @@ const createTab = async ({
     // If the text includes %c, then it probably came from the jest output being forwarded into the browser
     // So we don't need to print it _again_ in node, since it already came from node
     if (text.includes('%c')) return;
+    // This is intended so that transpilation errors from the module server,
+    // which will get a nice code frame in node,
+    // do not also log "Failed to load resource: the server responded with a status of 500"
+    if (
+      /Failed to load resource: the server responded with a status of 500/.test(
+        text,
+      ) &&
+      message.location().url?.includes(`http://localhost:${port}`)
+    )
+      return;
     const type = message.type();
     // Create a new console instance instead of using the global one
     // Because the global one is overridden by Jest, and it adds a misleading second stack trace and code frame below it
@@ -282,6 +293,7 @@ const createTab = async ({
 
   await page.goto(`http://localhost:${port}`);
 
+  /** Runs page.evaluate but it includes forgot-await detection */
   const safeEvaluate = async (
     caller: (...params: any) => any,
     ...args: Parameters<typeof page.evaluate>
@@ -304,9 +316,10 @@ const createTab = async ({
   const runJS: PleasantestUtils['runJS'] = async (code, args) => {
     // For some reason encodeURIComponent doesn't encode '
     const encodedCode = encodeURIComponent(code).replace(/'/g, '%27');
+    const buildStatus = createBuildStatusTracker();
     // This uses the testPath as the url so that if there are relative imports
     // in the inline code, the relative imports are resolved relative to the test file
-    const url = `http://localhost:${port}/${testPath}?inline-code=${encodedCode}`;
+    const url = `http://localhost:${port}/${testPath}?inline-code=${encodedCode}&build-id=${buildStatus.buildId}`;
     const res = (await safeEvaluate(
       runJS,
       new Function(
@@ -322,13 +335,14 @@ const createTab = async ({
       ) as () => any,
       ...(Array.isArray(args) ? (args as any) : []),
     )) as undefined | { message: string; stack: string };
+
+    const errorsFromBuild = buildStatus.complete();
+    // It only throws the first one but that is probably OK
+    if (errorsFromBuild) throw errorsFromBuild[0];
+
     if (res === undefined) return;
     if (typeof res !== 'object') throw res;
-    let { message, stack } = res;
-    if (message.includes(`Failed to fetch dynamically imported module: ${url}`))
-      message =
-        'Failed to load runJS code (most likely due to a transpilation error)';
-
+    const { message, stack } = res;
     const parsedStack = parseStackTrace(stack);
     const modifiedStack = parsedStack.map(async (stackItem) => {
       if (stackItem.raw.startsWith(stack.slice(0, stack.indexOf('\n'))))

src/module-server/build-status-tracker.ts

@@ -0,0 +1,37 @@
+// This makes it so that errors thrown by the module server get re-thrown inside of runJS/loadJS.
+// This is necessary because the network is between the runJS call and the module server, so errors do not propagate
+// By tracking which runJS/loadJS initiated the request for each file, the module server can know which runJS/loadJS needs to reject.
+// When runJS finishes, it will check to see if the buildStatuses map has any errors corresponding to its buildId.
+// If there are any errors, it throws the first one.
+
+const buildStatuses = new Map<number, Error[]>();
+
+let buildIds = 0;
+
+/**
+ * Add an error for a specific buildId.
+ * If the build is already finished (resolved),
+ * triggers an uncaught rejection, with the hopes that it will fail the test.
+ */
+export const rejectBuild = (buildId: number, error: Error) => {
+  const statusArray = buildStatuses.get(buildId);
+  if (statusArray) statusArray.push(error);
+  // Uncaught promise rejection!
+  // Hope that Jest will catch it and fail the test, otherwise it is just logged by Node
+  else Promise.reject(error);
+};
+
+export const createBuildStatusTracker = () => {
+  const buildId = ++buildIds;
+  buildStatuses.set(buildId, []);
+  return {
+    buildId,
+    complete() {
+      const status = buildStatuses.get(buildId);
+      // This should never happen
+      if (!status) throw new Error('Build already completed');
+      buildStatuses.delete(buildId);
+      if (status.length > 0) return status;
+    },
+  };
+};

src/module-server/error-with-location.ts

@@ -0,0 +1,49 @@
+import * as colors from 'kolorist';
+import { createCodeFrame } from 'simple-code-frame';
+import { promises as fs } from 'fs';
+
+export class ErrorWithLocation extends Error {
+  filename?: string;
+  line: number;
+  column?: number;
+  constructor({
+    message,
+    filename,
+    line,
+    column,
+  }: {
+    message: string;
+    filename?: string;
+    line: number;
+    column?: number;
+  }) {
+    super(message);
+    this.filename = filename;
+    this.line = line;
+    this.column = column;
+  }
+
+  async toCodeFrame() {
+    if (!this.filename)
+      throw new Error('filename missing in ErrorWithLocation');
+
+    const originalCode = await fs.readFile(this.filename, 'utf8');
+    const frame = createCodeFrame(
+      originalCode,
+      this.line - 1,
+      this.column || 0,
+    );
+    const message = `${colors.red(this.message)}
+
+${colors.blue(
+  `${this.filename}:${this.line}${
+    this.column === undefined ? '' : `:${this.column + 1}`
+  }`,
+)}
+
+${frame}`;
+    const modifiedError = new Error(message);
+    modifiedError.stack = message;
+    return modifiedError;
+  }
+}

src/module-server/index.ts

@@ -48,7 +48,7 @@ export const createModuleServer = async ({
     else normalPlugins.push(plugin);
   }
 
-  const plugins: (Plugin | false | undefined)[] = [
+  const plugins: Plugin[] = [
     ...prePlugins,
 
     ...normalPlugins,
@@ -57,19 +57,19 @@ export const createModuleServer = async ({
     environmentVariablesPlugin(envVars),
     npmPlugin({ root, envVars }),
 
-    esbuildOptions && esbuildPlugin(esbuildOptions),
+    ...(esbuildOptions ? [esbuildPlugin(esbuildOptions)] : []),
     cssPlugin({ root }),
 
     ...postPlugins,
   ];
-  const filteredPlugins = plugins.filter(Boolean) as Plugin[];
   const requestCache = new Map<string, SourceDescription>();
   const middleware: polka.Middleware[] = [
     indexHTMLMiddleware,
-    jsMiddleware({ root, plugins: filteredPlugins, requestCache }),
+    jsMiddleware({ root, plugins, requestCache }),
     cssMiddleware({ root }),
     staticMiddleware({ root }),
   ];
+
   return {
     ...(await createServer({ middleware })),
     requestCache,

src/module-server/middleware/js.ts

@@ -1,6 +1,10 @@
 import { dirname, posix, relative, resolve, sep } from 'path';
 import type polka from 'polka';
-import type { SourceDescription } from 'rollup';
+import type {
+  PartialResolvedId,
+  ResolveIdResult,
+  SourceDescription,
+} from 'rollup';
 import type { Plugin } from '../plugin';
 import { createPluginContainer } from '../rollup-plugin-container';
 import { promises as fs } from 'fs';
@@ -11,29 +15,54 @@ import type {
 } from '@ampproject/remapping/dist/types/types';
 import MagicString from 'magic-string';
 import { jsExts } from '../extensions-and-detection';
-import * as esbuild from 'esbuild';
-import { createCodeFrame } from 'simple-code-frame';
-import * as colors from 'kolorist';
-import { Console } from 'console';
+import { rejectBuild } from '../build-status-tracker';
+import { ErrorWithLocation } from '../error-with-location';
 
 interface JSMiddlewareOpts {
   root: string;
   plugins: Plugin[];
   requestCache: Map<string, SourceDescription>;
 }
 
+const getResolveCacheKey = (spec: string, from: string) =>
+  `${spec}%%FROM%%${from}`;
+
 // Minimal version of https://github.com/preactjs/wmr/blob/main/packages/wmr/src/wmr-middleware.js
 
 export const jsMiddleware = ({
   root,
   plugins,
   requestCache,
 }: JSMiddlewareOpts): polka.Middleware => {
+  interface ResolveCacheEntry {
+    buildId: number;
+    resolved: PartialResolvedId;
+  }
+  /**
+   * The resolve cache is used so that if something has already been resolved from a previous build,
+   * the buildId from the previous build gets used rather than the current buildId.
+   * That way, modules can get correctly deduped in the browser,
+   * and syntax/transform errors will get thrown from the _first_ runJS/loadJS that they were imported from.
+   */
+  const resolveCache = new Map<string, ResolveCacheEntry>();
+
+  const setInResolveCache = (
+    spec: string,
+    from: string,
+    buildId: number,
+    resolved: PartialResolvedId,
+  ) => resolveCache.set(getResolveCacheKey(spec, from), { buildId, resolved });
+
+  const getFromResolveCache = (spec: string, from: string) =>
+    resolveCache.get(getResolveCacheKey(spec, from));
+
   const rollupPlugins = createPluginContainer(plugins);
 
   rollupPlugins.buildStart();
 
   return async (req, res, next) => {
+    const buildId =
+      req.query['build-id'] !== undefined && Number(req.query['build-id']);
     try {
       // Normalized path starting with slash
       const path = posix.normalize(req.path);
@@ -53,6 +82,7 @@ export const jsMiddleware = ({
         const params = new URLSearchParams(req.query as Record<string, string>);
         params.delete('import');
         params.delete('inline-code');
+        params.delete('build-id');
 
         // Remove trailing =
         // This is necessary for rollup-plugin-vue, which ads ?lang.ts at the end of the id,
@@ -111,14 +141,37 @@ export const jsMiddleware = ({
       // Resolve all the imports and replace them, and inline the resulting resolved paths
       // This makes different ways of importing the same path (e.g. extensionless imports, etc.)
       // all dedupe to the same module so it is only executed once
-      code = await transformImports(code, id, {
+      code = await transformImports(code, id, map, {
         async resolveId(spec) {
+          const addBuildId = (specifier: string) => {
+            const delimiter = /\?/.test(specifier) ? '&' : '?';
+            return `${specifier}${delimiter}build-id=${localBuildId}`;
+          };
+
+          // Default to the buildId corresponding to this module
+          // But for any module which has previously been imported from another buildId,
+          // Use the previous buildId (for module deduplication in the browser)
+          let localBuildId = buildId;
           if (/^(data:|https?:|\/\/)/.test(spec)) return spec;
 
-          const resolved = await rollupPlugins.resolveId(spec, file);
+          const cached = getFromResolveCache(spec, file);
+          let resolved: ResolveIdResult;
+          if (cached) {
+            resolved = cached.resolved;
+            localBuildId = cached.buildId;
+          } else {
+            resolved = await rollupPlugins.resolveId(spec, file);
+            if (resolved && buildId)
+              setInResolveCache(
+                spec,
+                file,
+                buildId,
+                typeof resolved === 'object' ? resolved : { id: resolved },
+              );
+          }
           if (resolved) {
             spec = typeof resolved === 'object' ? resolved.id : resolved;
-            if (spec.startsWith('@npm/')) return `/${spec}`;
+            if (spec.startsWith('@npm/')) return addBuildId(`/${spec}`);
             if (/^(\/|\\|[a-z]:\\)/i.test(spec)) {
               // Change FS-absolute paths to relative
               spec = relative(dirname(file), spec).split(sep).join(posix.sep);
@@ -130,20 +183,20 @@ export const jsMiddleware = ({
 
               spec = relative(root, spec).split(sep).join(posix.sep);
               if (!/^(\/|[\w-]+:)/.test(spec)) spec = `/${spec}`;
-              return spec;
+              return addBuildId(spec);
             }
           }
 
-          // If it wasn't resovled, and doesn't have a js-like extension
-          // add the ?import query param so it is clear
+          // If it wasn't resolved, and doesn't have a js-like extension
+          // add the ?import query param to make it clear
           // that the request needs to end up as JS that can be imported
           if (!jsExts.test(spec)) {
             // If there is already a query parameter, add &import
             const delimiter = /\?/.test(spec) ? '&' : '?';
-            return `${spec}${delimiter}import`;
+            return addBuildId(`${spec}${delimiter}import`);
           }
 
-          return spec;
+          return addBuildId(spec);
         },
       });
 
@@ -153,29 +206,18 @@ export const jsMiddleware = ({
         'Content-Length': Buffer.byteLength(code, 'utf-8'),
       });
       res.end(code);
-
-      // Start a esbuild build (just for the sake of parsing)
-      // That way, if there is a parsing error in the code resulting from the rollup transforms,
-      // we can display an error/code frame in the console
-      // instead of just a generic message from the browser saying it couldn't parse
-      // We are *not awaiting* this because we don't want to slow down sending the HTTP response
-      esbuild.transform(code, { loader: 'js' }).catch((error) => {
-        const err = error.errors[0];
-        const { line, column } = err.location;
-        const frame = createCodeFrame(code as string, line - 1, column);
-        const message = `${colors.red(colors.bold(err.text))}
-
-${colors.red(`${id}:${line}:${(column as number) + 1}`)}
-
-${frame}
-`;
-
-        // Create a new console instance instead of using the global one
-        // Because the global one is overridden by Jest, and it adds a misleading second stack trace and code frame below it
-        const console = new Console(process.stdout, process.stderr);
-        console.error(message);
-      });
     } catch (error) {
+      if (buildId) {
+        rejectBuild(
+          Number(buildId),
+          error instanceof ErrorWithLocation
+            ? await error.toCodeFrame().catch(() => error)
+            : error,
+        );
+
+        res.statusCode = 500;
+        return res.end();
+      }
       next(error);
     }
   };