From e30b22d6237dbdbe67a15db2030e4ce95ce3d252 Mon Sep 17 00:00:00 2001 From: alexander-akait <4567934+alexander-akait@users.noreply.github.com> Date: Sat, 26 Sep 2026 16:02:39 +0000 Subject: [PATCH 1/4] feat(hot): ask less of a custom transport MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six required methods, of which three were only required because of how `createHot` happened to call them. Two of those are not a transport's job at all: `handler` is meaningless unless the transport is served over HTTP. The built-in WebSocket one carried it only to answer 426 to a plain request, which is the right answer for any transport that does not serve requests — so that is the default now, and the WebSocket stream no longer has a `handler` of its own. `hasClients` exists to skip building a payload nobody will read. That is an optimization a transport can make inside `publish`, and both built-ins already do. A transport that does not offer it is asked to publish rather than assumed to have no clients. Both stay accepted, so a transport written against 8.3.0 keeps working, and a TODO marks them for removal in the next major. What is required is now `onConnect`, `publish`, `publishTo` and `close` — and the validation names that shorter set when something is missing. Left alone deliberately: the client side keeps its class-constructor shape. It is what `client.webSocketTransport` has always been, so every custom client written for webpack-dev-server works here untouched; symmetry with the factory is not worth breaking those. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA --- .changeset/simpler-transport-contract.md | 5 ++ README.md | 30 +++++++--- src/hot.js | 48 ++++++++++----- src/servers/WebSocketServer.js | 15 ++--- test/hot.test.js | 75 +++++++++++++++++++++++- types/hot.d.ts | 8 +-- 6 files changed, 140 insertions(+), 41 deletions(-) create mode 100644 .changeset/simpler-transport-contract.md diff --git a/.changeset/simpler-transport-contract.md b/.changeset/simpler-transport-contract.md new file mode 100644 index 000000000..3f6a5c893 --- /dev/null +++ b/.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 diff --git a/README.md b/README.md index 0329fa8e0..849118f12 100644 --- a/README.md +++ b/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,9 +374,6 @@ middleware(compiler, { publishTo(client, payload) {}, // End every client and stop any timers. close() {}, - // Optional, for transports built on an upgrade. - attach(server) {}, - detach() {}, }), }, }); @@ -389,6 +381,26 @@ middleware(compiler, { 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`: ```ts diff --git a/src/hot.js b/src/hot.js index 1ef73202b..7439376a7 100644 --- a/src/hot.js +++ b/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,17 +108,33 @@ 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", - "handler", - "hasClients", - "onConnect", - "publish", - "publishTo", -]; +// 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"]; + +// 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} stream what a `transport` function returned @@ -557,7 +573,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 +662,7 @@ function createHot(compiler, userOptions, statsOption) { return; } - eventStream.handler(req, res); + (eventStream.handler || upgradeRequired)(req, res); }, publish(payload) { if (closed) return; diff --git a/src/servers/WebSocketServer.js b/src/servers/WebSocketServer.js index d51d3b9eb..27b52df31 100644 --- a/src/servers/WebSocketServer.js +++ b/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; }, diff --git a/test/hot.test.js b/test/hot.test.js index 0b8ff0214..ce69a4b27 100644 --- a/test/hot.test.js +++ b/test/hot.test.js @@ -1189,7 +1189,10 @@ describe("createHot over a WebSocket", () => { // Nothing but an upgrade can speak this transport, so a client which asked // for the path over plain HTTP is told so instead of being left hanging. + // The built-in WebSocket stream has no `handler` of its own — this is the + // answer `createHot` gives for any transport that does not serve requests. expect(response.status).toBe(426); + expect(await response.text()).toBe("Upgrade Required"); }); it("stops answering upgrades once closed", async () => { @@ -1314,7 +1317,7 @@ describe("createHot over a transport of your own", () => { expect(() => createHot(compiler, { transport: () => ({ publish() {} }) }), ).toThrow( - "The 'hot.transport' function must return a client stream, which is missing: close, handler, hasClients, onConnect, publishTo.", + "The 'hot.transport' function must return a client stream, which is missing: close, onConnect, publishTo.", ); }); @@ -1322,7 +1325,75 @@ describe("createHot over a transport of your own", () => { const compiler = makeFakeCompiler(); expect(() => createHot(compiler, { transport: () => undefined })).toThrow( - /must return a client stream, which is missing: close, handler/, + /must return a client stream, which is missing: close, onConnect/, ); }); + + it("accepts a transport that only does what a transport must", () => { + const compiler = makeFakeCompiler(); + /** @type {EXPECTED_ANY[]} */ + const published = []; + + // `handler` and `hasClients` are not a transport's job: one is meaningless + // unless it is served over HTTP, the other an optimization it can make + // inside `publish`. + const hot = createHot(compiler, { + transport: () => ({ + close() {}, + onConnect() {}, + publish(payload) { + published.push(payload); + }, + publishTo() {}, + }), + }); + + hot.publish({ action: "built" }); + + expect(published).toEqual([{ action: "built" }]); + + hot.close(); + }); + + it("publishes progress to a transport that does not answer hasClients", () => { + const compiler = makeFakeCompiler(); + /** @type {EXPECTED_OBJECT} */ + let progressHandler; + /** @type {EXPECTED_ANY[]} */ + const published = []; + + compiler.webpack = { + ProgressPlugin: class { + /** @param {EXPECTED_OBJECT} handler handler */ + constructor(handler) { + progressHandler = handler; + } + + apply() {} + }, + }; + + const hot = createHot(compiler, { + progress: true, + transport: () => ({ + close() {}, + onConnect() {}, + publish(payload) { + published.push(payload); + }, + publishTo() {}, + }), + }); + + progressHandler(0.25, "building"); + + // `hasClients` is what skips building a payload nobody will read. A + // transport that does not offer it is asked to publish and decides for + // itself, rather than being treated as having no clients at all. + expect(published).toEqual([ + { action: "progress", percent: 25, message: "building" }, + ]); + + hot.close(); + }); }); diff --git a/types/hot.d.ts b/types/hot.d.ts index 45f563556..8fab8655e 100644 --- a/types/hot.d.ts +++ b/types/hot.d.ts @@ -250,13 +250,13 @@ type StreamClient = ServerResponse | WebSocketLikeClient; */ type ClientStream = { /** - * answer a request on the endpoint's path + * answer a request on the endpoint's path; without one a request there is answered `426 Upgrade Required` */ - handler: (req: IncomingMessage, res: ServerResponse) => void; + handler?: ((req: IncomingMessage, res: ServerResponse) => void) | undefined; /** - * true when at least one client is connected + * true when at least one client is connected; without one a payload is built even if nobody is listening */ - hasClients: () => boolean; + hasClients?: (() => boolean) | undefined; /** * called with each client once it has joined */ From 0cb53c72f2a3205fd62341c0787adf5a76cb4c3a Mon Sep 17 00:00:00 2001 From: alexander-akait <4567934+alexander-akait@users.noreply.github.com> Date: Sat, 26 Sep 2026 16:22:17 +0000 Subject: [PATCH 2/4] fix(hot): validate the optional methods, and keep `handler` a method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings, both regressions from making the two methods optional. Dropping them from the required list stopped them being type-checked at all, so `handler: true` passed startup and threw from the endpoint instead — exactly the late, far-from-the-option failure this validation exists to prevent. Present and not callable is now rejected by name, while absent stays fine. `(eventStream.handler || upgradeRequired)(req, res)` also called the handler unbound. A stream written as an object literal reaching for `this` worked before this change, so the call goes back to being a method call and the default is a separate branch. Every other call on the stream was already a method call, so only this one had lost its receiver. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA --- src/hot.js | 31 ++++++++++++++++++++- test/hot.test.js | 70 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 1 deletion(-) diff --git a/src/hot.js b/src/hot.js index 7439376a7..e5012345c 100644 --- a/src/hot.js +++ b/src/hot.js @@ -113,6 +113,16 @@ function pathMatch(url, expected) { // 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", +]; + // 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 @@ -156,6 +166,18 @@ function checkClientStream(stream) { ); } + const notFunctions = OPTIONAL_CLIENT_STREAM_METHODS.filter((method) => { + const value = /** @type {Record} */ (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; } @@ -662,7 +684,14 @@ function createHot(compiler, userOptions, statsOption) { return; } - (eventStream.handler || upgradeRequired)(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; diff --git a/test/hot.test.js b/test/hot.test.js index ce69a4b27..201752fa6 100644 --- a/test/hot.test.js +++ b/test/hot.test.js @@ -1355,6 +1355,76 @@ describe("createHot over a transport of your own", () => { hot.close(); }); + it("rejects an optional method that is not callable", () => { + const compiler = makeFakeCompiler(); + + // Absent is the supported case; present and not callable would pass + // startup and throw from the endpoint instead, which is the failure this + // validation exists to prevent. + expect(() => + createHot(compiler, { + transport: () => ({ + close() {}, + onConnect() {}, + publish() {}, + publishTo() {}, + handler: true, + }), + }), + ).toThrow( + "The 'hot.transport' function returned a client stream whose optional method is not callable: handler.", + ); + }); + + it("names every optional method that is not callable", () => { + const compiler = makeFakeCompiler(); + + expect(() => + createHot(compiler, { + transport: () => ({ + close() {}, + onConnect() {}, + publish() {}, + publishTo() {}, + handler: true, + hasClients: 1, + }), + }), + ).toThrow(/optional methods are not callable: handler, hasClients\./); + }); + + it("calls a transport's handler as its own method", () => { + const compiler = makeFakeCompiler(); + /** @type {EXPECTED_ANY} */ + let receiver; + + const hot = createHot(compiler, { + transport: () => ({ + name: "mine", + close() {}, + onConnect() {}, + publish() {}, + publishTo() {}, + // Reaching for `this` is how a stream written as an object literal + // gets at its own state, and it worked before `handler` was optional. + handler(req, res) { + receiver = this.name; + res.end(); + }, + }), + }); + + hot.handle( + /** @type {EXPECTED_OBJECT} */ ({ url: "/__webpack_hmr" }), + /** @type {EXPECTED_OBJECT} */ ({ end() {} }), + () => {}, + ); + + expect(receiver).toBe("mine"); + + hot.close(); + }); + it("publishes progress to a transport that does not answer hasClients", () => { const compiler = makeFakeCompiler(); /** @type {EXPECTED_OBJECT} */ From d2f38e89d140623c84761d4f6fb39a180eacdafd Mon Sep 17 00:00:00 2001 From: alexander-akait <4567934+alexander-akait@users.noreply.github.com> Date: Sat, 26 Sep 2026 16:56:20 +0000 Subject: [PATCH 3/4] test(overlay): stop a second reload racing the runtime-slot assertion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI surfaced this on an unrelated PR: "Execution context was destroyed, most likely because of a navigation". The fixture accepts no updates, so every rebuild is a full reload, and the watcher can see one write twice — file timestamp granularity is enough. The second reload lands inside the `evaluate` that triggers the next error and takes its execution context with it. Nothing in the assertion is wrong; it just had no guarantee the page had stopped moving. `settle()` exists for this and is now awaited before the page is touched again. The test predates the transport work on this branch — it came in with #2370 — and is not reachable from anything that work changes, but it fails this branch's CI, so it is fixed here rather than re-run. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA --- test/e2e/overlay.test.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/test/e2e/overlay.test.js b/test/e2e/overlay.test.js index b60a9db58..a17c0112c 100644 --- a/test/e2e/overlay.test.js +++ b/test/e2e/overlay.test.js @@ -429,8 +429,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 From 703ab0a66a11c9663cef6d696bd157910b25b283 Mon Sep 17 00:00:00 2001 From: alexander-akait <4567934+alexander-akait@users.noreply.github.com> Date: Sat, 26 Sep 2026 17:39:08 +0000 Subject: [PATCH 4/4] test(overlay): read the recovered overlay without losing it to the reload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The red `Client` job, properly diagnosed this time. The failing test is "turns an error overlay into a warning overlay on partial recovery", not the one the previous commit hardened — I had inferred it from the line number in the stack, and for a puppeteer error jest points at the blank line between tests, so the name was the only reliable signal and I had grepped it away. The race: recovering from a build that failed cannot be applied hot, so the page reloads. `waitForFunction` survives that, because puppeteer re-installs it in the new document, but the `page.evaluate` that read the overlay afterwards does not — and dies with "Execution context was destroyed" when the reload lands between the two. Reproduced locally at roughly one run in ten. Waiting and reading are now one step, through a `waitForOverlayText` helper that returns the text the wait matched, with a retry for the narrower race where the navigation arrives after it resolved. The assertion still reads the text, so a missing "WARNING" fails as a comparison rather than a timeout. Twenty consecutive runs of the case, and no occurrence of the error. The `settle()` added to "resets the runtime slot on a clean build" in the previous commit stays: that test has the same reload hazard, so guarding it is right even though it was not what CI was failing on. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA --- test/e2e/overlay.test.js | 27 +++++++-------------- test/helpers/e2e.js | 51 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 18 deletions(-) diff --git a/test/e2e/overlay.test.js b/test/e2e/overlay.test.js index a17c0112c..768f88f67 100644 --- a/test/e2e/overlay.test.js +++ b/test/e2e/overlay.test.js @@ -9,6 +9,7 @@ import { waitForAppText, waitForNoOverlay, waitForOverlay, + waitForOverlayText, waitForRuntimeListeners, warningApp, } from "../helpers/e2e"; @@ -528,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 () => { diff --git a/test/helpers/e2e.js b/test/helpers/e2e.js index 1eac7db36..96580aac2 100644 --- a/test/helpers/e2e.js +++ b/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} 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} resolved once the overlay is gone @@ -203,6 +253,7 @@ module.exports = { waitForAppText, waitForNoOverlay, waitForOverlay, + waitForOverlayText, waitForRuntimeListeners, waitForText, warningApp,