diff --git a/.changeset/connected-log-both-transports.md b/.changeset/connected-log-both-transports.md new file mode 100644 index 000000000..d298e8e86 --- /dev/null +++ b/.changeset/connected-log-both-transports.md @@ -0,0 +1,5 @@ +--- +"webpack-dev-middleware": patch +--- + +Say `connected` whichever transport the client used, rather than only Server-Sent Events diff --git a/.changeset/overlay-focus.md b/.changeset/overlay-focus.md new file mode 100644 index 000000000..a143a467d --- /dev/null +++ b/.changeset/overlay-focus.md @@ -0,0 +1,5 @@ +--- +"webpack-dev-middleware": patch +--- + +Move focus into the error overlay when it opens, keep it on the navigation while paging through problems, and give it back to whatever the page had focused — a control inside an open shadow root included — when it closes, and give its frame an accessible name diff --git a/.changeset/transport-parity.md b/.changeset/transport-parity.md new file mode 100644 index 000000000..d15e56568 --- /dev/null +++ b/.changeset/transport-parity.md @@ -0,0 +1,5 @@ +--- +"webpack-dev-middleware": patch +--- + +Make the two client transports behave alike: neither logs a raw connection error, and neither reports anything after being closed diff --git a/client-src/clients/EventSourceClient.js b/client-src/clients/EventSourceClient.js index 3f2296075..2a8a9a2eb 100644 --- a/client-src/clients/EventSourceClient.js +++ b/client-src/clients/EventSourceClient.js @@ -1,5 +1,3 @@ -import { log } from "../utils/log.js"; - /** @typedef {import("./createSocket.js").CommunicationClient} CommunicationClient */ /** @typedef {import("./createSocket.js").ClientHandler} ClientHandler */ @@ -10,6 +8,10 @@ const DEFAULT_TIMEOUT = 20 * 1000; * Server-Sent Events. A connection can die without the browser firing `error` * — a proxy that stops forwarding, a laptop that slept — so this one watches * for silence as well, and reports that as a close for the caller to reconnect. + * + * A failure is not logged here, for the same reason the WebSocket one does not + * log it: `error` fires on every routine reconnection, so saying so would be + * noise rather than news. * @implements {CommunicationClient} */ export default class EventSourceClient { @@ -33,8 +35,11 @@ export default class EventSourceClient { this.client = new window.EventSource(url); this.client.addEventListener("open", () => { + if (this.closed) { + return; + } + this.lastActivity = Date.now(); - log.info("connected"); if (this.openHandler) { this.openHandler(); @@ -42,6 +47,10 @@ export default class EventSourceClient { }); this.client.addEventListener("message", (event) => { + if (this.closed) { + return; + } + this.lastActivity = Date.now(); if (this.messageHandler) { @@ -65,7 +74,6 @@ export default class EventSourceClient { * End this connection and report it, once. */ handleDisconnect() { - /* istanbul ignore next -- @preserve reached only by an event queued before close() */ if (this.closed) { return; } diff --git a/client-src/clients/WebSocketClient.js b/client-src/clients/WebSocketClient.js index 7f0efb0d6..8b7bda39b 100644 --- a/client-src/clients/WebSocketClient.js +++ b/client-src/clients/WebSocketClient.js @@ -1,5 +1,3 @@ -import { log } from "../utils/log.js"; - /** @typedef {import("./createSocket.js").CommunicationClient} CommunicationClient */ /** @typedef {import("./createSocket.js").ClientHandler} ClientHandler */ @@ -30,6 +28,11 @@ function toWebSocketURL(url) { * A WebSocket. The browser reports a dropped connection itself, and the server * pings to find a half-open one, so unlike Server-Sent Events this needs no * watchdog of its own. + * + * A failure is not logged here. The `error` event carries no detail by + * specification, so it would print an opaque object, and it is followed by the + * `close` the shared socket already reports and acts on — which is also all + * Server-Sent Events say, so neither transport is noisier than the other. * @implements {CommunicationClient} */ export default class WebSocketClient { @@ -37,9 +40,42 @@ export default class WebSocketClient { * @param {string} url url to connect to */ constructor(url) { + /** @type {ClientHandler | undefined} */ + this.openHandler = undefined; + /** @type {ClientHandler | undefined} */ + this.closeHandler = undefined; + /** @type {ClientHandler | undefined} */ + this.messageHandler = undefined; + // Set once closed, so an event the socket had already queued cannot report + // anything after the caller asked for none. + this.closed = false; + this.client = new WebSocket(toWebSocketURL(url)); - this.client.onerror = (error) => { - log.error(error); + + this.client.onopen = () => { + if (this.openHandler) { + this.openHandler(); + } + }; + + this.client.onclose = () => { + if (this.closed) { + return; + } + + if (this.closeHandler) { + this.closeHandler(); + } + }; + + this.client.onmessage = (event) => { + if (this.closed) { + return; + } + + if (this.messageHandler) { + this.messageHandler(event.data); + } }; } @@ -47,34 +83,28 @@ export default class WebSocketClient { * @param {ClientHandler} fn called once the connection is open */ onOpen(fn) { - this.client.onopen = () => { - fn(); - }; + this.openHandler = fn; } /** * @param {ClientHandler} fn called once the connection is gone */ onClose(fn) { - this.client.onclose = () => { - fn(); - }; + this.closeHandler = fn; } /** * @param {ClientHandler} fn called with each message, as a string */ onMessage(fn) { - this.client.onmessage = (event) => { - fn(event.data); - }; + this.messageHandler = fn; } /** * Close without reporting it, so the caller does not reconnect. */ close() { - this.client.onclose = null; + this.closed = true; this.client.close(); } } diff --git a/client-src/clients/createSocket.js b/client-src/clients/createSocket.js index b7888227b..4d8e3b86e 100644 --- a/client-src/clients/createSocket.js +++ b/client-src/clients/createSocket.js @@ -68,6 +68,9 @@ export default function createSocket(Client, url, options = {}) { client = new Client(url, options.clientOptions); client.onOpen(() => { + // Said here rather than in a transport, or whichever one did not say it + // would leave the page with no sign it had connected at all. + log.info("connected"); attempt = 0; }); diff --git a/client-src/overlay.js b/client-src/overlay.js index 74b749216..3a02ceff5 100644 --- a/client-src/overlay.js +++ b/client-src/overlay.js @@ -127,7 +127,9 @@ const colors = { * @property {HTMLElement | null} card visible panel inside the iframe * @property {boolean} runtimeListenersAttached whether the window listeners are attached * @property {boolean} hostKeydownAttached whether the host document's Escape listener is attached + * @property {boolean} focusOnRender whether the next render is the first one of a newly opened overlay * @property {number} pageIndex page shown when paginating + * @property {Element | null} previousActiveElement what the page had focused before the overlay opened * @property {Record} problemsBySource each reporting source's problems * @property {{ type: "errors" | "warnings", lines: string[] } | null} currentProblems union of every source, as displayed * @property {{ createHTML: (value: string) => EXPECTED_ANY } | undefined} trustedTypesPolicy trusted types policy @@ -141,7 +143,9 @@ function createOverlayState() { card: null, runtimeListenersAttached: false, hostKeydownAttached: false, + focusOnRender: false, pageIndex: 0, + previousActiveElement: null, problemsBySource: {}, currentProblems: null, trustedTypesPolicy: undefined, @@ -436,6 +440,23 @@ export function clear(source) { (state.frame.parentNode).removeChild(state.frame); } + // Hand focus back to whatever had it, or the page is left with focus on a + // removed element and the next Tab starts from the top of the document. + const { previousActiveElement } = state; + + if ( + previousActiveElement && + typeof (/** @type {HTMLElement} */ (previousActiveElement).focus) === + "function" && + // Removed from the document while the overlay was up: focusing it does + // nothing useful, and reading `isConnected` is how to tell. + previousActiveElement.isConnected !== false + ) { + /** @type {HTMLElement} */ (previousActiveElement).focus(); + } + + state.previousActiveElement = null; + state.focusOnRender = false; state.frame = null; state.card = null; state.problemsBySource = {}; @@ -482,8 +503,32 @@ function ensureOverlay() { ); } + // Whatever the page had focused, so it can be given back — the overlay + // takes focus to be reachable by keyboard, and is rude if it keeps it. + // `document.activeElement` names the shadow host rather than the control + // inside it, and focusing a host that does not delegate focus does nothing + // at all, so descend to the control itself. A closed root reports no + // `activeElement`, and there the host really is all there is to go back to. + let previouslyFocused = document.activeElement; + + while ( + previouslyFocused && + previouslyFocused.shadowRoot && + previouslyFocused.shadowRoot.activeElement + ) { + previouslyFocused = previouslyFocused.shadowRoot.activeElement; + } + + state.previousActiveElement = previouslyFocused; + // Only the render that opens the overlay takes focus. Paginating re-renders + // it, and moving focus then would take it off the button being clicked. + state.focusOnRender = true; + state.frame = document.createElement("iframe"); state.frame.id = OVERLAY_ID; + // An iframe with no accessible name is announced by its url, which here is + // `about:blank`. + state.frame.title = "Build errors and warnings"; state.frame.src = "about:blank"; applyStyle(state.frame, backdropStyles); document.body.appendChild(state.frame); @@ -579,6 +624,17 @@ function renderProblems() { const { type, lines } = state.currentProblems; const paginated = paginate && lines.length > 1; + // Emptying the card destroys whatever it had focused, and the browser then + // falls back to the frame's body — so paging with the keyboard would lose + // the button being used after the very first press. Which control it was is + // remembered here and given back below, since the rebuilt one is a + // different element. + const previouslyFocused = frameDocument.activeElement; + const refocus = + previouslyFocused && card.contains(previouslyFocused) + ? previouslyFocused.getAttribute("data-control") + : null; + // Accent the top bar with the problem color (red for errors, yellow for warnings). card.style.borderTopColor = `#${problemColor(type)}`; setHTML(card, ""); @@ -588,6 +644,8 @@ function renderProblems() { closeButton.type = "button"; closeButton.textContent = "×"; closeButton.setAttribute("aria-label", "Close"); + // Names this control across the re-render that replaces it, for `refocus`. + closeButton.setAttribute("data-control", "close"); applyStyle(closeButton, closeButtonStyles); closeButton.addEventListener("click", () => { clear(); @@ -595,6 +653,8 @@ function renderProblems() { card.appendChild(closeButton); const visible = paginated ? [lines[state.pageIndex]] : lines; + /** @type {{ previous?: HTMLButtonElement, next?: HTMLButtonElement }} */ + const navButtons = {}; if (paginated) { // Header row: badge and the problem's first line (usually the file @@ -641,13 +701,15 @@ function renderProblems() { * @param {string} text button text * @param {number} delta page delta * @param {string} ariaLabel accessible label + * @param {string} control name kept across re-renders, for `refocus` * @returns {HTMLButtonElement} nav button */ - const makeNavButton = (text, delta, ariaLabel) => { + const makeNavButton = (text, delta, ariaLabel, control) => { const button = frameDocument.createElement("button"); button.type = "button"; button.textContent = text; button.setAttribute("aria-label", ariaLabel); + button.setAttribute("data-control", control); applyStyle(button, { border: "none", background: "transparent", @@ -668,9 +730,17 @@ function renderProblems() { counter.textContent = `${state.pageIndex + 1} / ${lines.length}`; applyStyle(counter, { color: "#f2f2f2" }); - nav.appendChild(makeNavButton("‹", -1, "Previous problem")); + navButtons.previous = makeNavButton( + "‹", + -1, + "Previous problem", + "previous", + ); + navButtons.next = makeNavButton("›", 1, "Next problem", "next"); + + nav.appendChild(navButtons.previous); nav.appendChild(counter); - nav.appendChild(makeNavButton("›", 1, "Next problem")); + nav.appendChild(navButtons.next); header.appendChild(badge); header.appendChild(nav); card.appendChild(header); @@ -712,6 +782,21 @@ function renderProblems() { ? "Use ‹ › or the arrow keys to navigate. Click outside, press Esc, or fix the code to dismiss." : "Click outside, press Esc, or fix the code to dismiss."; card.appendChild(hint); + + // Focus reaches into the frame so Escape and the arrow keys work without a + // click first, and so a screen reader lands on the problem rather than + // staying where the page was. + if (state.focusOnRender) { + state.focusOnRender = false; + closeButton.focus(); + } else if (refocus) { + // The same control when this render still has one — a problem set that + // shrank back to a single page has no navigation left — and the close + // button otherwise, so focus stays inside the card either way. + ( + navButtons[/** @type {"previous" | "next"} */ (refocus)] || closeButton + ).focus(); + } } render = renderProblems; diff --git a/test/client-socket.test.js b/test/client-socket.test.js index da7b1b6b3..7f3c696a6 100644 --- a/test/client-socket.test.js +++ b/test/client-socket.test.js @@ -168,6 +168,25 @@ describe("createSocket", () => { expect(instances).toHaveLength(21); }); + it("backs off exponentially by default", () => { + const { FakeClient, instances } = createFakeClient(); + + // No `retryDelay`: the default doubles each time, so a server that is + // down does not get hammered once a second forever. + createSocket(FakeClient, "ws://localhost/hmr"); + + instances[0].closeHandler(); + jest.advanceTimersByTime(1100); + expect(instances).toHaveLength(2); + + instances[1].closeHandler(); + // The second wait is past two seconds, which the first was not. + jest.advanceTimersByTime(1100); + expect(instances).toHaveLength(2); + jest.advanceTimersByTime(1000); + expect(instances).toHaveLength(3); + }); + it("says it is reconnecting while the attempts are bounded", () => { const info = jest.spyOn(globalThis.console, "info").mockImplementation(); const { FakeClient, instances } = createFakeClient(); @@ -209,6 +228,41 @@ describe("createSocket", () => { info.mockRestore(); }); + it("takes an explicit choice about announcing retries", () => { + const info = jest.spyOn(globalThis.console, "info").mockImplementation(); + const { FakeClient, instances } = createFakeClient(); + + // Bounded attempts would announce themselves, but the caller said not to. + createSocket(FakeClient, "ws://localhost/hmr", { + logRetries: false, + retries: 2, + retryDelay: () => 1000, + }); + + instances[0].closeHandler(); + + expect(info).not.toHaveBeenCalledWith( + expect.stringContaining("Trying to reconnect"), + ); + + info.mockRestore(); + }); + + it("closes cleanly while waiting to reconnect", () => { + const { FakeClient, instances } = createFakeClient(); + const socket = createSocket(FakeClient, "ws://localhost/hmr", { + retryDelay: () => 1000, + }); + + // Closed with no live connection, only a pending timer — there is nothing + // to close, and the timer must not fire afterwards. + instances[0].closeHandler(); + socket.close(); + jest.advanceTimersByTime(10000); + + expect(instances).toHaveLength(1); + }); + it("stops reconnecting once closed", () => { const { FakeClient, instances } = createFakeClient(); const socket = createSocket(FakeClient, "ws://localhost/hmr", { diff --git a/test/client-transports.test.js b/test/client-transports.test.js new file mode 100644 index 000000000..938779446 --- /dev/null +++ b/test/client-transports.test.js @@ -0,0 +1,313 @@ +import EventSourceClient from "../client-src/clients/EventSourceClient"; +import WebSocketClient from "../client-src/clients/WebSocketClient"; + +// eslint-disable-next-line jsdoc/reject-any-type +/** @typedef {any} EXPECTED_OBJECT */ + +/** + * Stand in for the browser's `EventSource`, driven from the test. + * @returns {EXPECTED_OBJECT} the constructor and what it built + */ +function fakeEventSource() { + /** @type {EXPECTED_OBJECT[]} */ + const instances = []; + + globalThis.window = /** @type {EXPECTED_OBJECT} */ ({ + EventSource: function EventSource(url) { + /** @type {EXPECTED_OBJECT} */ + const source = { + url, + closed: false, + listeners: {}, + addEventListener(type, fn) { + source.listeners[type] = fn; + }, + close() { + source.closed = true; + }, + emit(type, event) { + if (source.listeners[type]) { + source.listeners[type](event); + } + }, + }; + + instances.push(source); + + return source; + }, + }); + + return instances; +} + +/** + * Stand in for the browser's `WebSocket`, plus the anchor the url resolver uses. + * @returns {EXPECTED_OBJECT} the sockets built + */ +function fakeWebSocket() { + /** @type {EXPECTED_OBJECT[]} */ + const instances = []; + + globalThis.document = /** @type {EXPECTED_OBJECT} */ ({ + createElement: () => { + /** @type {EXPECTED_OBJECT} */ + const anchor = {}; + + Object.defineProperty(anchor, "href", { + get: () => anchor.resolved, + set: (value) => { + anchor.resolved = /^[a-z]+:\/\//i.test(value) + ? value + : `https://example.test${value}`; + }, + }); + + return anchor; + }, + }); + + globalThis.WebSocket = /** @type {EXPECTED_OBJECT} */ ( + function WebSocket(url) { + /** @type {EXPECTED_OBJECT} */ + const socket = this; + + socket.url = url; + socket.closed = false; + socket.close = () => { + socket.closed = true; + }; + + instances.push(socket); + } + ); + + return instances; +} + +/** + * The two transports answer the same calls, so the cases below are written + * once and run against each. `emit` is how the test plays the browser. + */ +const transports = [ + { + name: "EventSourceClient", + Client: EventSourceClient, + setup: fakeEventSource, + teardown: () => { + delete globalThis.window; + }, + // `EventSource` has no `close` event — a dropped connection is an `error`, + // which is exactly the difference this shared contract hides. + emit: (instance, type, event) => + instance.emit(type === "close" ? "error" : type, event), + }, + { + name: "WebSocketClient", + Client: WebSocketClient, + setup: fakeWebSocket, + teardown: () => { + delete globalThis.document; + delete globalThis.WebSocket; + }, + emit: (instance, type, event) => { + const handler = { + open: "onopen", + close: "onclose", + message: "onmessage", + }; + + if (instance[handler[type]]) { + instance[handler[type]](event); + } + }, + }, +]; + +for (const { name, Client, setup, teardown, emit } of transports) { + describe(`${name} (the transport contract)`, () => { + /** @type {EXPECTED_OBJECT[]} */ + let instances; + + beforeEach(() => { + // `EventSourceClient` arms a watchdog in its constructor, and the cases + // below that never close their client would otherwise leave a real + // interval ticking after the file is done. + jest.useFakeTimers(); + instances = setup(); + }); + + afterEach(() => { + teardown(); + jest.useRealTimers(); + }); + + it("reports the connection opening", () => { + const client = new Client("/__webpack_hmr"); + const opened = jest.fn(); + + client.onOpen(opened); + emit(instances[0], "open", {}); + + expect(opened).toHaveBeenCalledTimes(1); + }); + + it("reports the connection closing", () => { + const client = new Client("/__webpack_hmr"); + const closed = jest.fn(); + + client.onClose(closed); + emit(instances[0], "close", {}); + + expect(closed).toHaveBeenCalledTimes(1); + }); + + it("hands the message on as a string", () => { + const client = new Client("/__webpack_hmr"); + const received = jest.fn(); + + client.onMessage(received); + emit(instances[0], "message", { data: '{"action":"built"}' }); + + expect(received).toHaveBeenCalledWith('{"action":"built"}'); + }); + + it("tolerates an event arriving before anything is listening", () => { + const client = new Client("/__webpack_hmr"); + + // The caller registers its handlers after constructing, so an event in + // between must not throw. + expect(client).toBeDefined(); + expect(() => { + emit(instances[0], "open", {}); + emit(instances[0], "message", { data: "{}" }); + emit(instances[0], "close", {}); + }).not.toThrow(); + }); + + it("closes the underlying connection", () => { + const client = new Client("/__webpack_hmr"); + + client.close(); + + expect(instances[0].closed).toBe(true); + }); + + it("says nothing after being closed", () => { + const client = new Client("/__webpack_hmr"); + const closed = jest.fn(); + const received = jest.fn(); + + client.onClose(closed); + client.onMessage(received); + client.close(); + + // Whatever the browser had already queued must not reach the caller + // after it asked for none — or the shared socket reconnects a + // connection nobody wants. + emit(instances[0], "close", {}); + emit(instances[0], "message", { data: "{}" }); + + expect(closed).not.toHaveBeenCalled(); + expect(received).not.toHaveBeenCalled(); + }); + }); +} + +describe("EventSourceClient (what only it does)", () => { + /** + * @param {EXPECTED_OBJECT} instance fake source + * @param {string} type event type + * @param {EXPECTED_OBJECT} event event + */ + function emit(instance, type, event) { + instance.emit(type, event); + } + + /** @type {EXPECTED_OBJECT[]} */ + let instances; + + beforeEach(() => { + jest.useFakeTimers(); + instances = fakeEventSource(); + }); + + afterEach(() => { + delete globalThis.window; + jest.useRealTimers(); + }); + + it("reports a close when the connection falls silent", () => { + const client = new EventSourceClient("/__webpack_hmr", { timeout: 1000 }); + const closed = jest.fn(); + + client.onClose(closed); + + // A connection can die without the browser firing `error` at all, so + // silence past the timeout is the only thing that notices. The watchdog + // ticks twice per timeout, so the first tick past it is at 1500ms. + jest.advanceTimersByTime(1500); + + expect(closed).toHaveBeenCalledTimes(1); + expect(instances[0].closed).toBe(true); + }); + + it("keeps the connection while messages keep arriving", () => { + const client = new EventSourceClient("/__webpack_hmr", { timeout: 1000 }); + const closed = jest.fn(); + + client.onClose(closed); + + for (let i = 0; i < 4; i++) { + jest.advanceTimersByTime(600); + emit(instances[0], "message", { data: "💓" }); + } + + expect(closed).not.toHaveBeenCalled(); + }); + + it("reports a close once, however it was noticed", () => { + const client = new EventSourceClient("/__webpack_hmr", { timeout: 1000 }); + const closed = jest.fn(); + + client.onClose(closed); + instances[0].emit("error", {}); + // The watchdog is still armed when the error arrives; both must not + // report, or the shared socket schedules two reconnections. + jest.advanceTimersByTime(5000); + instances[0].emit("error", {}); + + expect(closed).toHaveBeenCalledTimes(1); + }); + + it("stops the watchdog when closed", () => { + const client = new EventSourceClient("/__webpack_hmr", { timeout: 1000 }); + const closed = jest.fn(); + + client.onClose(closed); + client.close(); + jest.advanceTimersByTime(60000); + + expect(closed).not.toHaveBeenCalled(); + }); + + it("ignores an open that arrives after it was closed", () => { + const client = new EventSourceClient("/__webpack_hmr", { timeout: 1000 }); + const opened = jest.fn(); + + client.onOpen(opened); + client.close(); + + // A connection that completed its handshake as the caller closed it would + // otherwise look open, and reset the watchdog on a dead source. + instances[0].emit("open", {}); + + expect(opened).not.toHaveBeenCalled(); + }); + + it("falls back to a default timeout", () => { + const client = new EventSourceClient("/__webpack_hmr"); + + expect(client.timeout).toBe(20000); + }); +}); diff --git a/test/e2e/overlay.test.js b/test/e2e/overlay.test.js index 37cb7f4cb..b143ed6f4 100644 --- a/test/e2e/overlay.test.js +++ b/test/e2e/overlay.test.js @@ -184,6 +184,90 @@ describe("error overlay (browser)", () => { expect(await page.$(`#${OVERLAY_ID}`)).toBeNull(); }); + it("takes focus when it opens and hands it back when it closes", async () => { + hotApp = await createHotApp({ code: acceptedApp("v1") }); + ({ page, browser } = await runBrowser()); + + await page.goto(hotApp.url); + await waitForAppText(page, "v1"); + + // Something on the page holds focus before the overlay appears. + await page.evaluate(() => { + const input = document.createElement("input"); + + input.id = "focus-me"; + document.body.appendChild(input); + input.focus(); + }); + + hotApp.edit("broken while focused {{{"); + const frame = await waitForOverlay(page); + + // Focus reaches into the frame, so Escape and the arrow keys work without + // clicking first, and a screen reader lands on the problem. + expect( + await frame.evaluate(() => + document.activeElement + ? document.activeElement.getAttribute("aria-label") + : null, + ), + ).toBe("Close"); + + // The frame is announced by name rather than by its `about:blank` url. + expect( + await page.$eval(`#${OVERLAY_ID}`, (element) => element.title), + ).toBeTruthy(); + + await page.keyboard.press("Escape"); + await waitForNoOverlay(page); + + // Focus goes back where it was, rather than being lost with the removed + // frame and sending the next Tab to the top of the document. + expect( + await page.evaluate(() => + document.activeElement ? document.activeElement.id : null, + ), + ).toBe("focus-me"); + }); + + it("hands focus back to a control inside a shadow root", async () => { + hotApp = await createHotApp({ code: acceptedApp("v1") }); + ({ page, browser } = await runBrowser()); + + await page.goto(hotApp.url); + await waitForAppText(page, "v1"); + + await page.evaluate(() => { + const host = document.createElement("div"); + + document.body.appendChild(host); + + const input = document.createElement("input"); + + input.id = "in-shadow"; + host.attachShadow({ mode: "open" }).appendChild(input); + input.focus(); + }); + + hotApp.edit("broken while a shadow control is focused {{{"); + await waitForOverlay(page); + await page.keyboard.press("Escape"); + await waitForNoOverlay(page); + + // `document.activeElement` names the host rather than the control, and a + // host that does not delegate focus cannot be focused at all — so without + // descending into the root first, focus is simply dropped here. + expect( + await page.evaluate(() => { + const active = document.activeElement; + const inner = + active && active.shadowRoot && active.shadowRoot.activeElement; + + return inner ? inner.id : null; + }), + ).toBe("in-shadow"); + }); + it("dismisses on backdrop and close-button clicks, but not inside the card", async () => { hotApp = await createHotApp({ code: acceptedApp("v1") }); ({ page, browser } = await runBrowser()); @@ -615,6 +699,53 @@ describe("error overlay (browser)", () => { ); }); + it("keeps focus on the navigation across a page change", async () => { + hotApp = await createHotApp({ + code: ` + try { + require("./a"); + } catch (err) { + // expected + } + try { + require("./b"); + } catch (err) { + // expected + } + `, + files: { + "a.js": "broken a {{{", + "b.js": "broken b {{{", + }, + }); + ({ page, browser } = await runBrowser()); + + await page.goto(hotApp.url); + + const frame = await waitForOverlay(page); + + await frame.waitForFunction(() => + document.body.textContent.includes("1 / 2"), + ); + await frame.click('[aria-label="Previous problem"]'); + await frame.click('[aria-label="Next problem"]'); + await frame.waitForFunction(() => + document.body.textContent.includes("2 / 2"), + ); + + // Turning the page rebuilds the card, which destroys the very button that + // was clicked. Handing focus to its replacement is what keeps a keyboard + // user on the navigation rather than dropping them at the top of the card + // after the first press. + expect( + await frame.evaluate(() => + document.activeElement + ? document.activeElement.getAttribute("aria-label") + : null, + ), + ).toBe("Next problem"); + }); + it("shows the full problem list when paginate=false", async () => { hotApp = await createHotApp({ query: '?overlay={"paginate":false}', @@ -847,6 +978,133 @@ describe("error overlay (browser)", () => { }); }); +describe("error overlay parity with webpack-dev-server (browser)", () => { + let hotApp; + let browser; + let page; + + afterEach(async () => { + ({ browser, app: hotApp } = await closeE2e(browser, hotApp)); + }); + + it("keeps an error the filter accepts", async () => { + hotApp = await createHotApp({ + query: + '?overlay={"errors":"function(message){return message.includes(`keep-me`)}"}', + code: acceptedApp("v1"), + }); + ({ page, browser } = await runBrowser()); + + await page.goto(hotApp.url); + await waitForAppText(page, "v1"); + + hotApp.edit("keep-me is not valid javascript {{{"); + const frame = await waitForOverlay(page); + + expect(await frame.evaluate(() => document.body.textContent)).toContain( + "keep-me", + ); + }); + + it("shows no overlay when the errors filter rejects everything", async () => { + hotApp = await createHotApp({ + query: '?overlay={"errors":"function(){return false}"}', + code: acceptedApp("v1"), + }); + ({ page, browser } = await runBrowser()); + const console_ = collectConsole(page); + + await page.goto(hotApp.url); + await waitForAppText(page, "v1"); + + hotApp.edit("rejected by the filter {{{"); + // The build problem still reaches the console — just not the DOM. + await console_.waitFor("Module parse failed"); + + expect(await page.$(`#${OVERLAY_ID}`)).toBeNull(); + }); + + it("shows an error raised by the very first build", async () => { + // Broken before the browser ever connects, so the overlay has to come + // from the catch-up sync rather than from a rebuild. + hotApp = await createHotApp({ code: "broken from the start {{{" }); + ({ page, browser } = await runBrowser()); + + await page.goto(hotApp.url); + const frame = await waitForOverlay(page); + + expect(await frame.evaluate(() => document.body.textContent)).toContain( + "Module parse failed", + ); + }); + + it("replaces the overlay when a rebuild is still broken", async () => { + hotApp = await createHotApp({ code: acceptedApp("v1") }); + ({ page, browser } = await runBrowser()); + + await page.goto(hotApp.url); + await waitForAppText(page, "v1"); + + hotApp.edit("first breakage {{{"); + let frame = await waitForOverlay(page); + + expect(await frame.evaluate(() => document.body.textContent)).toContain( + "first breakage", + ); + + // A second, different failure must replace the first rather than leaving + // the stale one on screen. + hotApp.edit("second breakage {{{"); + frame = await waitForOverlay(page); + await frame.waitForFunction(() => + document.body.textContent.includes("second breakage"), + ); + + expect(await frame.evaluate(() => document.body.textContent)).not.toContain( + "first breakage", + ); + }); + + it("dismisses with Escape more than once", async () => { + hotApp = await createHotApp({ code: acceptedApp("v1") }); + ({ page, browser } = await runBrowser()); + + await page.goto(hotApp.url); + await waitForAppText(page, "v1"); + + hotApp.edit("broken once {{{"); + await waitForOverlay(page); + await page.keyboard.press("Escape"); + await waitForNoOverlay(page); + + // Dismissing must not tear down whatever lets the next problem re-open + // it — a listener removed for good would leave the page blind. + hotApp.edit("broken twice {{{"); + await waitForOverlay(page); + await page.keyboard.press("Escape"); + await waitForNoOverlay(page); + + expect(await page.$(`#${OVERLAY_ID}`)).toBeNull(); + }); + + it("escapes markup in a build error instead of rendering it", async () => { + hotApp = await createHotApp({ code: acceptedApp("v1") }); + ({ page, browser } = await runBrowser()); + + await page.goto(hotApp.url); + await waitForAppText(page, "v1"); + + // The error text carries markup, which must reach the card as text. + hotApp.edit("const bad = \"\" {{{"); + const frame = await waitForOverlay(page); + + expect( + await frame.evaluate(() => document.querySelectorAll("img").length), + ).toBe(0); + expect(await page.evaluate(() => globalThis.xss)).toBeUndefined(); + }); +}); + describe("overlay shared state across bundled copies (browser)", () => { const OVERLAY_ENTRY = require.resolve("../../client-src/overlay.js"); const OVERLAY_STATE_KEY = "__webpack_dev_middleware_hot_overlay_state__"; diff --git a/test/e2e/transport.test.js b/test/e2e/transport.test.js new file mode 100644 index 000000000..f1cad4b15 --- /dev/null +++ b/test/e2e/transport.test.js @@ -0,0 +1,147 @@ +import collectConsole from "../helpers/console-collector"; +import { + OVERLAY_ID, + acceptedApp, + closeE2e, + unacceptedApp, + waitForAppText, + waitForNoOverlay, + waitForOverlay, +} from "../helpers/e2e"; +import createHotApp from "../helpers/hot-app"; +import runBrowser from "../helpers/run-browser"; + +jest.setTimeout(400000); + +// Everything below is the same story over each transport. The client and the +// server agree on which one through matching options, and nothing above them +// is supposed to be able to tell the difference — which is exactly the claim +// worth testing rather than assuming. +for (const transport of ["sse", "ws"]) { + describe(`hot client over ${transport} (browser)`, () => { + let hotApp; + let browser; + let page; + + afterEach(async () => { + ({ browser, app: hotApp } = await closeE2e(browser, hotApp)); + }); + + it("connects and applies a hot update", async () => { + hotApp = await createHotApp({ transport, code: acceptedApp("v1") }); + ({ page, browser } = await runBrowser()); + const console_ = collectConsole(page); + + await page.goto(hotApp.url); + await waitForAppText(page, "v1"); + await console_.waitFor("connected"); + + // A marker that survives HMR but not a navigation, so the update below + // is proved to have been applied rather than reloaded into place. + await page.evaluate(() => { + globalThis.notReloaded = true; + }); + + hotApp.edit(acceptedApp("v2")); + await waitForAppText(page, "v2"); + + expect(await page.evaluate(() => globalThis.notReloaded)).toBe(true); + }); + + it("falls back to a reload when the update cannot be applied", async () => { + hotApp = await createHotApp({ transport, code: unacceptedApp("v1") }); + ({ page, browser } = await runBrowser()); + + await page.goto(hotApp.url); + await waitForAppText(page, "v1"); + + await page.evaluate(() => { + globalThis.notReloaded = true; + }); + + hotApp.edit(unacceptedApp("v2")); + await waitForAppText(page, "v2"); + + // Nothing accepted the update, so the page went round again. + expect(await page.evaluate(() => globalThis.notReloaded)).toBeUndefined(); + }); + + it("shows a build error in the overlay and clears it on recovery", async () => { + hotApp = await createHotApp({ transport, code: acceptedApp("v1") }); + ({ page, browser } = await runBrowser()); + + await page.goto(hotApp.url); + await waitForAppText(page, "v1"); + + hotApp.edit("this is not valid javascript {{{"); + const frame = await waitForOverlay(page); + + expect(await frame.evaluate(() => document.body.textContent)).toContain( + "Module parse failed", + ); + + // Recovering from a broken build cannot be applied hot, so the page + // reloads — wait for it to settle before reading the DOM, or the query + // races the navigation. + hotApp.edit(acceptedApp("v2")); + await waitForAppText(page, "v2"); + await waitForNoOverlay(page); + + expect(await page.$(`#${OVERLAY_ID}`)).toBeNull(); + }); + + it("reconnects after the server restarts", async () => { + hotApp = await createHotApp({ + transport, + query: "?timeout=1000", + code: acceptedApp("v1"), + // Heartbeats faster than the shortened timeout, so the inactivity + // watchdog does not churn disconnect/reconnect cycles mid-test. + hot: { heartbeat: 300 }, + }); + ({ page, browser } = await runBrowser()); + const console_ = collectConsole(page); + + await page.goto(hotApp.url); + await waitForAppText(page, "v1"); + await console_.waitFor("connected"); + + // Rebuilt while the server is down, so the update can only have arrived + // through the catch-up sync a reconnected client is sent. Restarting + // has to hand the new server to the transport again — a WebSocket + // upgrade is answered by the server, not by the middleware. + await hotApp.stopHttp(); + + const rebuilt = hotApp.nextBuild(); + + hotApp.edit(acceptedApp("v2")); + await rebuilt; + await hotApp.startHttp(); + + await waitForAppText(page, "v2"); + + expect( + await page.evaluate(() => document.getElementById("app").textContent), + ).toContain("v2"); + }); + + it("catches a client up on what it missed while it was away", async () => { + hotApp = await createHotApp({ transport, code: acceptedApp("v1") }); + ({ page, browser } = await runBrowser()); + + await page.goto(hotApp.url); + await waitForAppText(page, "v1"); + + // Broken before the page is opened, so the overlay can only come from + // the catch-up a newly connected client is sent. + hotApp.edit("broken before connecting {{{"); + await page.reload(); + + const frame = await waitForOverlay(page); + + expect(await frame.evaluate(() => document.body.textContent)).toContain( + "Module parse failed", + ); + }); + }); +} diff --git a/test/helpers/hot-app.js b/test/helpers/hot-app.js index 171be5799..9931b87e7 100644 --- a/test/helpers/hot-app.js +++ b/test/helpers/hot-app.js @@ -112,7 +112,11 @@ function makeConfig( * app becomes a named compilation whose client connects with `?name=` * and renders from `.js`. `pageHeaders` are sent with the HTML page * (e.g. a Content-Security-Policy). - * @param {{ query?: string, code?: string, files?: Record, apps?: { name: string, code: string }[], hot?: EXPECTED_ANY, stats?: EXPECTED_ANY, pageHeaders?: Record, publicPath?: string, setup?: (server: EXPECTED_ANY) => void, hmrPlugin?: boolean }} options options + * Pass `transport: "ws"` to serve the events over a WebSocket instead of + * Server-Sent Events: the middleware is told to, the client is asked for the + * matching transport, and the HTTP server is handed over so it can answer the + * upgrade. + * @param {{ query?: string, code?: string, files?: Record, apps?: { name: string, code: string }[], hot?: EXPECTED_ANY, stats?: EXPECTED_ANY, pageHeaders?: Record, publicPath?: string, setup?: (server: EXPECTED_ANY) => void, hmrPlugin?: boolean, transport?: ("sse" | "ws") }} options options * @returns {Promise} handles for the running app */ async function createHotApp({ @@ -126,6 +130,7 @@ async function createHotApp({ publicPath = "/", setup, hmrPlugin = true, + transport = "sse", }) { const dir = fs.mkdtempSync( path.join(fs.realpathSync.native(os.tmpdir()), "wdm-e2e-"), @@ -152,6 +157,13 @@ async function createHotApp({ /** @type {string[]} */ let scripts; + // The client picks its transport from its own query, so it has to be told + // the same thing the middleware was. + const clientQuery = + transport === "ws" + ? `?transport=ws${query ? `&${query.replace(/^\?/, "")}` : ""}` + : query; + if (apps) { config = apps.map((app) => { // One context dir per compilation: editing one app's entry must not @@ -161,7 +173,7 @@ async function createHotApp({ fs.mkdirSync(appDir, { recursive: true }); entryFiles[app.name] = path.join(appDir, "entry.js"); fs.writeFileSync(entryFiles[app.name], app.code); - return makeConfig(app.name, appDir, entryFiles[app.name], query); + return makeConfig(app.name, appDir, entryFiles[app.name], clientQuery); }); scripts = apps.map((app) => `/${app.name}.js`); } else { @@ -171,7 +183,7 @@ async function createHotApp({ "", dir, entryFiles[""], - query, + clientQuery, publicPath, hmrPlugin, ); @@ -189,7 +201,13 @@ async function createHotApp({ } }); - instance = middleware(compiler, { hot, stats }); + instance = middleware(compiler, { + hot: + transport === "ws" && hot + ? { ...(hot === true ? {} : hot), transport: "ws" } + : hot, + stats, + }); const app = express(); @@ -227,7 +245,43 @@ async function createHotApp({ }); }); + /** @type {EXPECTED_ANY[]} */ + let upgradedSockets = []; + + /** + * A WebSocket handshake is an upgrade the HTTP server answers, which the + * middleware never sees — so it only works once the server is handed over. + * Every server this app listens on needs that again, the replacement one a + * restart brings up included. + * @param {EXPECTED_ANY} httpServer the server now serving this app + */ + const attachTransport = (httpServer) => { + if (transport !== "ws") { + return; + } + + instance.attach(httpServer); + // Once a socket is upgraded it stops being the HTTP server's to close, + // so `closeAllConnections()` does not reach it and a shutdown would + // wait on a connected client forever. Tracked here to be severed by + // hand, which is also what a client sees when a server really dies. + httpServer.on("upgrade", (/** @type {EXPECTED_ANY} */ _req, socket) => { + upgradedSockets.push(socket); + }); + }; + + /** Sever every upgraded connection this app is holding open. */ + const severUpgraded = () => { + for (const socket of upgradedSockets) { + socket.destroy(); + } + + upgradedSockets = []; + }; + server = await listen(0); + attachTransport(server); + const { port } = server.address(); await new Promise((resolve) => { @@ -303,6 +357,7 @@ async function createHotApp({ */ stopHttp() { return new Promise((resolve) => { + severUpgraded(); server.closeAllConnections(); server.close(() => resolve()); }); @@ -315,6 +370,7 @@ async function createHotApp({ */ async startHttp() { server = await listen(port); + attachTransport(server); }, /** @@ -329,6 +385,7 @@ async function createHotApp({ resolve(); return; } + severUpgraded(); server.closeAllConnections(); server.close(() => resolve()); });