feat(hot): ask less of a custom transport

.changeset/simpler-transport-contract.md

@@ -0,0 +1,5 @@
+---
+"webpack-dev-middleware": minor
+---
+
+A custom `hot.transport` now needs only four methods — `onConnect`, `publish`, `publishTo` and `close`. `handler` and `hasClients` became optional: without a `handler` a request on the endpoint's path is answered `426 Upgrade Required`, and without `hasClients` a payload is built and the transport decides for itself in `publish`. A transport that implements all six keeps working unchanged

README.md

@@ -354,7 +354,7 @@ instance.attach(server);
 
 A plain `GET` on the path under `'ws'` answers `426 Upgrade Required`.
 
-A **function** builds a transport of your own. It is called with the resolved `path` and `heartbeat` and a logger, and must return a client stream — the same calls the built-in two answer:
+A **function** builds a transport of your own. It is called with the resolved `path` and `heartbeat` and a logger, and must return a client stream. Four methods are required:
 
 ```js
 /**
@@ -365,11 +365,6 @@ A **function** builds a transport of your own. It is called with the resolved `p
 middleware(compiler, {
   hot: {
     transport: ({ path, heartbeat }, logger) => ({
-      // Answer a request on the endpoint's path.
-      handler(req, res) {},
-      // True while at least one client is connected; a compilation with no
-      // clients skips serializing its payload.
-      hasClients: () => clients.size > 0,
       // Call `fn` with each client once it has joined. It is what catches a
       // client up with the last hashes, so it can apply the next update.
       onConnect(fn) {},
@@ -379,16 +374,33 @@ middleware(compiler, {
       publishTo(client, payload) {},
       // End every client and stop any timers.
       close() {},
-      // Optional, for transports built on an upgrade.
-      attach(server) {},
-      detach() {},
     }),
   },
 });
 ```
 
 A function that returns something missing one of those throws, naming what is absent, rather than failing later from wherever it is first published to.
 
+Four more are optional:
+
+```js
+const transport = ({ path, heartbeat }, logger) => ({
+  // ...the four above, and any of these:
+
+  // Answer a request on the endpoint's path. Only a transport served over
+  // HTTP needs one; without it a request there is answered
+  // `426 Upgrade Required`, which is what the built-in WebSocket relies on.
+  handler(req, res) {},
+  // True while at least one client is connected, so a compilation with none
+  // can skip building its payload. Without it, `publish` is called and the
+  // transport decides for itself.
+  hasClients: () => clients.size > 0,
+  // For a transport built on an upgrade, which the middleware never sees.
+  attach(server) {},
+  detach() {},
+});
+```
+
 The clients are yours — whatever `onConnect` hands out is what `publishTo` takes back — so in TypeScript name their type through `ClientStreamFactory<T>`:
 
 ```ts

src/hot.js

@@ -64,8 +64,8 @@
  * in `publishTo`.
  * @template {EXPECTED_ANY} [TClient=StreamClient]
  * @typedef {object} ClientStream
- * @property {(req: IncomingMessage, res: ServerResponse) => void} handler answer a request on the endpoint's path
- * @property {() => boolean} hasClients true when at least one client is connected
+ * @property {((req: IncomingMessage, res: ServerResponse) => void)=} handler answer a request on the endpoint's path; without one a request there is answered `426 Upgrade Required`
+ * @property {(() => boolean)=} hasClients true when at least one client is connected; without one a payload is built even if nobody is listening
  * @property {(fn: (client: TClient) => void) => void} onConnect called with each client once it has joined
  * @property {(payload: Payload | { action: string }) => void} publish publish a payload to every client
  * @property {(client: TClient, payload: Payload | { action: string }) => void} publishTo publish a payload to a single client
@@ -108,18 +108,44 @@ function pathMatch(url, expected) {
   }
 }
 
-// Everything `createHot` calls on a stream. A transport of your own which is
-// missing one of them would throw from wherever it is first published to,
-// which is a long way from the option that built it.
-const CLIENT_STREAM_METHODS = [
-  "close",
+// What a transport has to do for itself. Missing one of these would throw from
+// wherever the stream is first published to, which is a long way from the
+// option that built it.
+const CLIENT_STREAM_METHODS = ["close", "onConnect", "publish", "publishTo"];
+
+// What it may also do. Absent is fine; present and not a function is not — that
+// would pass startup and throw from the endpoint or a progress callback later,
+// which is the failure being validated against in the first place.
+const OPTIONAL_CLIENT_STREAM_METHODS = [
+  "attach",
+  "detach",
   "handler",
   "hasClients",
-  "onConnect",
-  "publish",
-  "publishTo",
 ];
 
+// TODO remove in the next major release, along with the `handler` and
+// `hasClients` entries in `ClientStream`. Both were required of every transport
+// when `hot.transport` shipped, and neither needs to be: `handler` is
+// meaningless for a transport that is not served over HTTP — the built-in
+// WebSocket one only had it to answer 426 — and `hasClients` is an
+// optimization a transport can make inside `publish`. They stay optional
+// rather than being dropped now so a transport written against 8.3.0 keeps
+// working.
+//
+// Answers a request on the endpoint's path for a transport that has no
+// `handler`: reaching it over plain HTTP means the client cannot speak this
+// transport at all.
+/** @type {(req: IncomingMessage, res: ServerResponse) => void} */
+const upgradeRequired = (req, res) => {
+  if (!res.headersSent) {
+    res.writeHead(426, { "Content-Type": "text/plain; charset=utf-8" });
+  }
+
+  if (!res.writableEnded) {
+    res.end("Upgrade Required");
+  }
+};
+
 /**
  * @param {ClientStream<EXPECTED_ANY>} stream what a `transport` function returned
  * @returns {ClientStream<EXPECTED_ANY>} the same stream
@@ -140,6 +166,18 @@ function checkClientStream(stream) {
     );
   }
 
+  const notFunctions = OPTIONAL_CLIENT_STREAM_METHODS.filter((method) => {
+    const value = /** @type {Record<string, unknown>} */ (stream)[method];
+
+    return value !== undefined && typeof value !== "function";
+  });
+
+  if (notFunctions.length > 0) {
+    throw new TypeError(
+      `The 'hot.transport' function returned a client stream whose optional ${notFunctions.length === 1 ? "method is" : "methods are"} not callable: ${notFunctions.join(", ")}.`,
+    );
+  }
+
   return stream;
 }
 
@@ -557,7 +595,9 @@ function createHot(compiler, userOptions, statsOption) {
 
     // Published only when the rounded percent changes to keep the stream small.
     new webpack.ProgressPlugin((percent, message) => {
-      if (closed || !eventStream.hasClients()) {
+      // No `hasClients` means the transport did not offer to answer, so the
+      // payload is built and it decides in `publish`.
+      if (closed || (eventStream.hasClients && !eventStream.hasClients())) {
         return;
       }
 
@@ -644,7 +684,14 @@ function createHot(compiler, userOptions, statsOption) {
         return;
       }
 
-      eventStream.handler(req, res);
+      // Called as a method, not through a picked-off reference: a transport
+      // whose `handler` reaches for `this` was working before this became
+      // optional, and must keep working.
+      if (eventStream.handler) {
+        eventStream.handler(req, res);
+      } else {
+        upgradeRequired(req, res);
+      }
     },
     publish(payload) {
       if (closed) return;

src/servers/WebSocketServer.js

@@ -181,17 +181,10 @@ function createWebSocketStream({ path, heartbeat }, logger) {
       implementation.close();
     },
     detach,
-    handler(req, res) {
-      // The handshake is an upgrade the HTTP server answers, so a plain request
-      // reaching the middleware is a client which cannot speak this transport.
-      if (!res.headersSent) {
-        res.writeHead(426, { "Content-Type": "text/plain; charset=utf-8" });
-      }
-
-      if (!res.writableEnded) {
-        res.end("Upgrade Required");
-      }
-    },
+    // No `handler`: the handshake is an upgrade the HTTP server answers, so a
+    // plain request reaching the middleware is a client which cannot speak this
+    // transport, and answering that is `createHot`'s default for any transport
+    // that does not serve requests itself.
     hasClients() {
       return clients.size > 0;
     },

test/e2e/overlay.test.js

@@ -9,6 +9,7 @@ import {
   waitForAppText,
   waitForNoOverlay,
   waitForOverlay,
+  waitForOverlayText,
   waitForRuntimeListeners,
   warningApp,
 } from "../helpers/e2e";
@@ -429,8 +430,14 @@ describe("error overlay (browser)", () => {
     hotApp.edit(boomApp("v2"));
     await waitForAppText(page, "v2");
     await waitForNoOverlay(page);
-    // The fixture does not accept updates, so that was a full reload: the
-    // listeners are attached again, and racing them fails the same way.
+    // The fixture does not accept updates, so every rebuild is a full reload.
+    // A second one — the watcher seeing the same write twice, which file
+    // timestamp granularity makes possible — would land in the middle of the
+    // evaluate below and destroy its execution context, so wait until no
+    // further build is pending before touching the page again.
+    await hotApp.settle();
+    // Reloaded, so the listeners are attached afresh; racing them fails the
+    // same way.
     await waitForRuntimeListeners(page);
 
     // …so the next error starts a fresh slot instead of paging after the
@@ -522,24 +529,14 @@ describe("error overlay (browser)", () => {
           module.hot.accept();
         }
       `);
-    await page.waitForFunction(
-      (id) => {
-        const body = document.getElementById(id)?.contentDocument?.body;
-        return (
-          body &&
-          body.textContent.includes("Critical dependency") &&
-          !body.textContent.includes("Module parse failed")
-        );
-      },
-      { timeout: 30000, polling: 100 },
-      OVERLAY_ID,
-    );
-    expect(
-      await page.evaluate(
-        (id) => document.getElementById(id).contentDocument.body.textContent,
-        OVERLAY_ID,
-      ),
-    ).toContain("WARNING");
+    // Read by the wait itself: the recovery reloads the page, and a separate
+    // read afterwards loses its execution context to that navigation.
+    const text = await waitForOverlayText(page, {
+      includes: ["Critical dependency"],
+      excludes: ["Module parse failed"],
+    });
+
+    expect(text).toContain("WARNING");
   });
 
   it('overlay={"runtimeErrors":false} leaves runtime errors uncaught', async () => {

test/helpers/e2e.js

@@ -68,6 +68,56 @@ async function waitForOverlay(page) {
   return handle.contentFrame();
 }
 
+/**
+ * Wait until the overlay's text contains everything in `includes` and none of
+ * `excludes`, and return that text.
+ *
+ * Waiting and reading have to happen together. Recovering from a build that
+ * failed cannot be applied hot, so the page reloads — and while puppeteer
+ * re-installs a `waitForFunction` in the new document, a separate
+ * `page.evaluate` afterwards dies with "Execution context was destroyed" when
+ * the reload lands between the two. The retry covers the narrower race where
+ * the navigation arrives after the wait resolved but before its handle is read.
+ * @param {import("puppeteer").Page} page page
+ * @param {{ includes?: string[], excludes?: string[] }} text what the overlay's text must and must not contain
+ * @returns {Promise<string>} the overlay body's text once it matches
+ */
+async function waitForOverlayText(page, { includes = [], excludes = [] } = {}) {
+  for (let attempt = 0; ; attempt += 1) {
+    try {
+      const handle = await page.waitForFunction(
+        (id, want, avoid) => {
+          const body = document.getElementById(id)?.contentDocument?.body;
+
+          if (!body) {
+            return null;
+          }
+
+          const { textContent } = body;
+
+          return want.every((part) => textContent.includes(part)) &&
+            avoid.every((part) => !textContent.includes(part))
+            ? textContent
+            : null;
+        },
+        { polling: 100, timeout: 30000 },
+        OVERLAY_ID,
+        includes,
+        excludes,
+      );
+
+      return await handle.jsonValue();
+    } catch (error) {
+      if (
+        attempt >= 5 ||
+        !/Execution context was destroyed/.test(String(error))
+      ) {
+        throw error;
+      }
+    }
+  }
+}
+
 /**
  * @param {import("puppeteer").Page} page page
  * @returns {Promise<void>} resolved once the overlay is gone
@@ -203,6 +253,7 @@ module.exports = {
   waitForAppText,
   waitForNoOverlay,
   waitForOverlay,
+  waitForOverlayText,
   waitForRuntimeListeners,
   waitForText,
   warningApp,