feat(client): format one of webpack's problems, wherever it is read

.changeset/feat-problem-formatter.md

@@ -0,0 +1,33 @@
+---
+"webpack-dev-middleware": minor
+---
+
+The overlay now takes one of webpack's errors or warnings as it comes, not
+only a formatted string:
+
+```js
+import { showProblems } from "webpack-dev-middleware/client/overlay";
+
+showProblems("errors", stats.errors, "build");
+```
+
+The middleware formats its own payloads on the server, so its client never
+needed this. A server that sends webpack's error objects to the browser and
+formats them there — which is what webpack-dev-server does — had to write that
+formatting itself. It is `webpack-dev-middleware/client/problem` now, and
+`formatProblem` is there for a console as well as an overlay:
+
+```js
+import { formatProblem } from "webpack-dev-middleware/client/problem";
+
+const { header, body } = formatProblem("error", error);
+// "ERROR in ./src/app.js 3:0", "Module parse failed: ..."
+```
+
+The server's own formatting reads the same way as a result, which fixes two
+things it got wrong. An error webpack names no module for sent a first line
+holding a single space — and the overlay reads the first line as the heading,
+so it drew a heading with nothing in it; it now sends the message alone. And a
+module built by loaders reports its whole request (`babel-loader!./app.js`),
+which read as noise where the file is what matters; the file comes first now,
+with the request after it, and `file` is used when webpack sets one.

client-src/overlay.js

@@ -1,7 +1,13 @@
 import ansiHTML from "ansi-html-community";
 
+import { problemLine } from "./problem.js";
 import theme from "./theme.js";
 
+// Re-exported so a consumer that shows problems in the console as well as in
+// the overlay has one import for both. `./client/problem` is the same thing
+// without the overlay, for one that only formats.
+export { formatProblem, problemLine } from "./problem.js";
+
 // eslint-disable-next-line jsdoc/reject-any-type
 /** @typedef {any} EXPECTED_ANY */
 
@@ -820,23 +826,30 @@ render = renderProblems;
 
 /**
  * @param {"errors" | "warnings"} type problem type
- * @param {string[]} lines messages to render
+ * @param {(string | import("./problem.js").Problem)[]} problems what to
+ * render: a message, or one of webpack's errors or warnings as
+ * `stats.toJson()` reports it — a server that sends those to the browser
+ * rather than formatting them itself has nothing to convert
  * @param {string=} source who reports them — each source (e.g. this client,
  * the webpack-dev-server client, the runtime error capture) keeps its own
  * slot and the overlay renders the union of every slot
  */
-export function showProblems(type, lines, source = "") {
+export function showProblems(type, problems, source = "") {
   // Nothing to report is not something to report. Kept as a slot, it made the
   // union non-empty, which put an empty card on the page for the reader to
   // dismiss by hand — and, once another source cleared, left the page covered
   // by an overlay with no problems in it at all.
-  if (lines.length === 0) {
+  if (problems.length === 0) {
     clear(source);
 
     return;
   }
 
-  state.problemsBySource[source] = { type, lines: [...lines] };
+  const lines = problems.map((problem) =>
+    typeof problem === "string" ? problem : problemLine(problem),
+  );
+
+  state.problemsBySource[source] = { type, lines };
 
   const union =
     /** @type {{ type: "errors" | "warnings", lines: string[] }} */

client-src/problem.js

@@ -0,0 +1,123 @@
+// Turning one of webpack's problems into something to read.
+//
+// The middleware formats a build's errors and warnings on the server, so its
+// own payloads carry strings and its client never needs this. A server that
+// sends webpack's error objects to the browser instead — which is what
+// webpack-dev-server does — needs the same shape built there, and had to
+// write it itself. Here it is once.
+//
+// Compiled to an ES5 baseline like the rest of the browser runtime, so
+// nothing here is newer than that.
+
+// eslint-disable-next-line jsdoc/reject-any-type
+/** @typedef {any} EXPECTED_ANY */
+
+/**
+ * One of webpack's errors or warnings, as `stats.toJson()` reports it. Only
+ * the fields that say something about where and what; anything else on a
+ * `StatsError` is ignored.
+ * @typedef {object} Problem
+ * @property {string=} file the file the problem is in
+ * @property {string=} moduleName the module's request, loaders and all
+ * @property {string=} loc line and column within the module
+ * @property {string=} message what went wrong
+ * @property {(string[] | EXPECTED_ANY)=} stack frames webpack attached, when it did
+ */
+
+/**
+ * Where a problem happened, as one line.
+ *
+ * A module processed by loaders reports its whole request as `moduleName`
+ * (`babel-loader!./src/app.js`), which reads as noise where the file is what
+ * matters — so the file comes first and the request follows it in brackets.
+ * Empty when webpack said neither, rather than a line with nothing on it.
+ * @param {string | Problem} item a problem, or a message on its own
+ * @returns {string} the location, or an empty string
+ */
+export function problemLocation(item) {
+  if (typeof item === "string") {
+    return "";
+  }
+
+  const file = item.file || "";
+  const request = item.moduleName || "";
+  // `indexOf`, not `includes`: this file is compiled to an ES5 baseline.
+  const loaded = request.indexOf("!") !== -1;
+  // The module itself, with the loaders that built it stripped off.
+  const moduleName = loaded ? request.replace(/^(\s|\S)*!/, "") : request;
+
+  if (!moduleName && !file) {
+    return "";
+  }
+
+  let where = moduleName || file;
+
+  // The full request, when loaders made it something other than the module.
+  if (loaded) {
+    where += ` (${request})`;
+  }
+
+  // ... and the file, when webpack named one and it is not already what is
+  // shown. It usually names none, and when it does it is often the module
+  // itself — appending it either way read as `./a.js (./a.js)`.
+  if (moduleName && file && file !== moduleName) {
+    where += ` (${file})`;
+  }
+
+  return `${where}${item.loc ? ` ${item.loc}` : ""}`;
+}
+
+/**
+ * What a problem says, with any stack webpack attached under it.
+ * @param {string | Problem} item a problem, or a message on its own
+ * @returns {string} the message
+ */
+export function problemBody(item) {
+  if (typeof item === "string") {
+    return item;
+  }
+
+  let body = item.message || "";
+
+  if (Array.isArray(item.stack)) {
+    // `for...of` needs an array iterator, which an ES5 target does not have.
+    item.stack.forEach((frame) => {
+      if (typeof frame === "string") {
+        body += `\r\n${frame}`;
+      }
+    });
+  }
+
+  return body;
+}
+
+/**
+ * A problem as one string, which is what the overlay renders: the location on
+ * the first line, read as the heading, and the message under it.
+ * @param {string | Problem} item a problem, or a message on its own
+ * @returns {string} the problem
+ */
+export function problemLine(item) {
+  const location = problemLocation(item);
+  const body = problemBody(item);
+
+  return location ? `${location}\n${body}` : body;
+}
+
+/**
+ * A problem split for a console, where the level belongs in the heading rather
+ * than being drawn around it.
+ * @param {string} type `"warning"`, or anything else for an error
+ * @param {string | Problem} item a problem, or a message on its own
+ * @returns {{ header: string, body: string }} the problem, in two parts
+ */
+export function formatProblem(type, item) {
+  const location = problemLocation(item);
+
+  return {
+    header: `${type === "warning" ? "WARNING" : "ERROR"}${
+      location ? ` in ${location}` : ""
+    }`,
+    body: problemBody(item),
+  };
+}

package.json

@@ -41,6 +41,10 @@
       "types": "./types/client/overlay.d.ts",
       "default": "./client/overlay.js"
     },
+    "./client/problem": {
+      "types": "./types/client/problem.d.ts",
+      "default": "./client/problem.js"
+    },
     "./package.json": "./package.json"
   },
   "main": "dist/index.js",

src/hot.js

@@ -388,11 +388,36 @@ function formatErrors(errors) {
     return /** @type {string[]} */ (errors);
   }
 
+  // The same shape the browser runtime builds from an error object, so a
+  // problem reads the same whether this formatted it or a server sent the
+  // object over and `client/problem` did. Written out rather than shared:
+  // this half is CommonJS in node, that half is an ES module in a bundle.
   return /** @type {StatsError[]} */ (errors).map((error) => {
-    const moduleName = error.moduleName || "";
-    const loc = error.loc || "";
+    const file = error.file || "";
+    const request = error.moduleName || "";
+    const loaded = request.includes("!");
+    const moduleName = loaded ? request.replace(/^(\s|\S)*!/, "") : request;
 
-    return `${moduleName} ${loc}\n${error.message}`;
+    let where = moduleName || file;
+
+    if (where) {
+      if (loaded) {
+        where += ` (${request})`;
+      }
+
+      if (moduleName && file && file !== moduleName) {
+        where += ` (${file})`;
+      }
+
+      if (error.loc) {
+        where += ` ${error.loc}`;
+      }
+    }
+
+    // Nothing on the first line rather than a line holding a single space,
+    // which is what an error webpack names no module for used to send — and
+    // the overlay reads that first line as the heading.
+    return where ? `${where}\n${error.message}` : `${error.message}`;
   });
 }
 

test/e2e/overlay.test.js

@@ -1566,6 +1566,57 @@ describe("overlay shared state across bundled copies (browser)", () => {
     expect(await page.$(`#${OVERLAY_ID}`)).toBeNull();
   });
 
+  it("takes one of webpack's problems as it comes", async () => {
+    await start();
+    await page.goto(hotApp.url);
+
+    // A server that sends error objects to the browser rather than formatting
+    // them first has nothing to convert — which is the whole of what
+    // webpack-dev-server had to write for itself.
+    await page.evaluate(() => {
+      globalThis.overlayA.showProblems(
+        "errors",
+        [
+          {
+            moduleName: "babel-loader!./src/app.js",
+            loc: "3:11",
+            message: "Unexpected token",
+            stack: ["  at parse (babel)"],
+          },
+        ],
+        "a",
+      );
+    });
+
+    const frame = await overlayFrame();
+    const text = await frame.evaluate(() => document.body.textContent);
+
+    expect(text).toContain("./src/app.js");
+    expect(text).toContain("babel-loader!./src/app.js");
+    expect(text).toContain("3:11");
+    expect(text).toContain("Unexpected token");
+    expect(text).toContain("at parse (babel)");
+  });
+
+  it("mixes problems and plain messages in one set", async () => {
+    await start();
+    await page.goto(hotApp.url);
+
+    await page.evaluate(() => {
+      globalThis.overlayA.showProblems(
+        "errors",
+        ["a plain message", { moduleName: "./src/b.js", message: "an object" }],
+        "a",
+      );
+    });
+
+    const frame = await overlayFrame();
+
+    expect(await frame.evaluate(() => document.body.textContent)).toContain(
+      "1 / 2",
+    );
+  });
+
   it("dismisses on Escape pressed inside the overlay frame", async () => {
     await start();
     await page.goto(hotApp.url);

test/hot.test.js

@@ -1,5 +1,6 @@
 import http from "node:http";
 
+import { problemLine } from "../client-src/problem";
 import createHot, {
   createEventStream,
   formatErrors,
@@ -152,8 +153,55 @@ describe("hot middleware (unit)", () => {
       ).toEqual(["./foo.js 1:1\nboom"]);
     });
 
-    it("tolerates missing moduleName and loc", () => {
-      expect(formatErrors([{ message: "boom" }])).toEqual([" \nboom"]);
+    it("says only the message when webpack named no module", () => {
+      // Not `" \nboom"`: the overlay reads the first line as the heading, so
+      // a line holding a single space is a heading with nothing in it.
+      expect(formatErrors([{ message: "boom" }])).toEqual(["boom"]);
+      expect(formatErrors([{ loc: "main", message: "boom" }])).toEqual([
+        "boom",
+      ]);
+    });
+
+    it("puts the module first and the loaders that built it after", () => {
+      expect(
+        formatErrors([
+          { moduleName: "babel-loader!./foo.js", loc: "1:1", message: "boom" },
+        ]),
+      ).toEqual(["./foo.js (babel-loader!./foo.js) 1:1\nboom"]);
+    });
+
+    it("names the file as well, when webpack named a different one", () => {
+      expect(
+        formatErrors([
+          { moduleName: "./foo.js", file: "./bar.js", message: "boom" },
+        ]),
+      ).toEqual(["./foo.js (./bar.js)\nboom"]);
+    });
+
+    it("does not name the same file twice", () => {
+      expect(
+        formatErrors([
+          { moduleName: "./foo.js", file: "./foo.js", message: "boom" },
+        ]),
+      ).toEqual(["./foo.js\nboom"]);
+      expect(formatErrors([{ file: "./foo.js", message: "boom" }])).toEqual([
+        "./foo.js\nboom",
+      ]);
+    });
+
+    // The browser runtime builds the same shape from an error object a server
+    // sends over instead of formatting, so the same build has to read the
+    // same way either way round.
+    it("agrees with what the browser builds from the same error", () => {
+      const errors = [
+        { moduleName: "./foo.js", loc: "1:1", message: "boom" },
+        { moduleName: "babel-loader!./foo.js", message: "boom" },
+        { moduleName: "./foo.js", file: "./bar.js", message: "boom" },
+        { file: "./foo.js", message: "boom" },
+        { loc: "main", message: "boom" },
+      ];
+
+      expect(formatErrors(errors)).toEqual(errors.map(problemLine));
     });
   });