fix(proxy): improve HMR upgrade path handling and user proxy integration

Author: bjohansebas

test/server/proxy-option.test.js

@@ -859,134 +859,14 @@ describe("proxy option", () => {
     });
   });
 
-  describe("HMR upgrade path matching is exact when dispatching upgrades", () => {
+  describe("HMR upgrade dispatching to user proxies", () => {
     let server;
     let backend;
     let backendUpgradeCount;
 
-    beforeAll(async () => {
-      backendUpgradeCount = 0;
-
-      backend = http.createServer();
-      new WebSocketServer({ server: backend }).on("connection", () => {
-        backendUpgradeCount += 1;
-      });
-
-      await new Promise((resolve) => {
-        backend.listen(port5, resolve);
-      });
-
-      const compiler = webpack(config);
-
-      server = new Server(
-        {
-          hot: true,
-          allowedHosts: "all",
-          webSocketServer: "ws",
-          proxy: [
-            {
-              context: "/",
-              target: `http://localhost:${port5}`,
-              ws: true,
-            },
-          ],
-          port: port3,
-        },
-        compiler,
-      );
-
-      await server.start();
-    });
-
-    afterAll(async () => {
-      backend.closeAllConnections();
-      await server.stop();
-      await new Promise((resolve) => {
-        backend.close(resolve);
-      });
-    });
-
-    // The HMR server's default path is /ws. The dispatch matches the path
-    // exactly, the same way the underlying `ws` server (`shouldHandle`) does, so
-    // only the configured path is recognized as the HMR socket. The query
-    // string is stripped before comparison (again like `ws`), so `/ws?token=1`
-    // is still recognized as the HMR path.
-    const hmrVariants = [
-      ["exact path", "/ws"],
-      ["path with query string", "/ws?token=1"],
-    ];
-
-    it.each(hmrVariants)(
-      "treats %s (%s) as the HMR upgrade path (not forwarded to the proxy)",
-      async (_label, path) => {
-        const before = backendUpgradeCount;
-
-        const ws = new WebSocket(`ws://localhost:${port3}${path}`);
-        ws.on("error", () => {});
-
-        await new Promise((resolve) => {
-          setTimeout(resolve, 400);
-        });
-
-        try {
-          ws.close();
-        } catch {
-          // ignore close errors on already-failed sockets
-        }
-
-        await new Promise((resolve) => {
-          setTimeout(resolve, 200);
-        });
-
-        expect(backendUpgradeCount).toBe(before);
-      },
-    );
-
-    // Leading double-slash, case, trailing slash and percent-encoding are all
-    // significant: none equals the configured HMR path under the raw comparison
-    // `ws` uses, so they are forwarded to the user proxy (which is what `ws`
-    // would do — it refuses to serve them).
-    const proxiedVariants = [
-      ["leading double slash", "//ws"],
-      ["trailing slash", "/ws/"],
-      ["uppercase", "/WS"],
-      ["mixed case", "/wS"],
-      ["percent-encoded path", "/%77%73"],
-    ];
-
-    it.each(proxiedVariants)(
-      "forwards %s (%s) to the user proxy",
-      async (_label, path) => {
-        const before = backendUpgradeCount;
-
-        const ws = new WebSocket(`ws://localhost:${port3}${path}`);
-        ws.on("error", () => {});
-
-        await new Promise((resolve) => {
-          setTimeout(resolve, 400);
-        });
-
-        try {
-          ws.close();
-        } catch {
-          // ignore close errors on already-failed sockets
-        }
-
-        await new Promise((resolve) => {
-          setTimeout(resolve, 200);
-        });
-
-        expect(backendUpgradeCount).toBe(before + 1);
-      },
-    );
-  });
-
-  describe("HMR upgrade path matching honors a custom webSocketServer path", () => {
-    let server;
-    let backend;
-    let backendUpgradeCount;
-
-    beforeAll(async () => {
+    // Start a backend WebSocket server (the user proxy target) and a dev-server
+    // proxying everything to it, with the given dev-server options merged in.
+    const setup = async (devServerOptions) => {
       backendUpgradeCount = 0;
 
       backend = http.createServer();
@@ -998,13 +878,10 @@ describe("proxy option", () => {
         backend.listen(port5, resolve);
       });
 
-      const compiler = webpack(config);
-
       server = new Server(
         {
           hot: true,
           allowedHosts: "all",
-          webSocketServer: { type: "ws", options: { path: "/custom-hmr" } },
           proxy: [
             {
               context: "/",
@@ -1013,27 +890,33 @@ describe("proxy option", () => {
             },
           ],
           port: port3,
+          ...devServerOptions,
         },
-        compiler,
+        webpack(config),
       );
 
       await server.start();
-    });
+    };
 
-    afterAll(async () => {
+    const teardown = async () => {
       backend.closeAllConnections();
       await server.stop();
       await new Promise((resolve) => {
         backend.close(resolve);
       });
-    });
+    };
 
-    // The dispatch reads the HMR path from the configured `webSocketServer`
-    // options, not a hardcoded `/ws`.
-    it("treats the configured path (/custom-hmr) as the HMR upgrade path (not forwarded to the proxy)", async () => {
+    // Open a WebSocket to `path` and report whether the dev-server completed the
+    // handshake (`opened`) and whether the upgrade was forwarded to the backend
+    // proxy (`forwarded`).
+    const probe = async (path) => {
       const before = backendUpgradeCount;
 
-      const ws = new WebSocket(`ws://localhost:${port3}/custom-hmr`);
+      let opened = false;
+      const ws = new WebSocket(`ws://localhost:${port3}${path}`);
+      ws.on("open", () => {
+        opened = true;
+      });
       ws.on("error", () => {});
 
       await new Promise((resolve) => {
@@ -1050,106 +933,111 @@ describe("proxy option", () => {
         setTimeout(resolve, 200);
       });
 
-      expect(backendUpgradeCount).toBe(before);
-    });
+      return { opened, forwarded: backendUpgradeCount > before };
+    };
 
-    // The default `/ws` is no longer special once a custom path is configured,
-    // so it is forwarded to the user proxy like any other path.
-    it("forwards the default path (/ws) to the user proxy when it is not the configured HMR path", async () => {
-      const before = backendUpgradeCount;
+    // Behavior shared by every WebSocket server implementation: the HMR socket
+    // is served locally and never forwarded, while any path the HMR server does
+    // not own falls through to the user proxy. SockJS serves its transport under
+    // `/<prefix>/<server>/<session>/websocket`, not the bare `/ws`.
+    const serverTypes = [
+      { type: "ws", hmrPath: "/ws", nonHmrPath: "/not-hmr" },
+      {
+        type: "sockjs",
+        hmrPath: "/ws/000/abcd1234/websocket",
+        nonHmrPath: "/not-hmr",
+      },
+    ];
 
-      const ws = new WebSocket(`ws://localhost:${port3}/ws`);
-      ws.on("error", () => {});
+    for (const { type, hmrPath, nonHmrPath } of serverTypes) {
+      describe(`with webSocketServerType: ${type}`, () => {
+        beforeAll(() => setup({ webSocketServer: type }));
 
-      await new Promise((resolve) => {
-        setTimeout(resolve, 400);
-      });
+        afterAll(teardown);
 
-      try {
-        ws.close();
-      } catch {
-        // ignore close errors on already-failed sockets
-      }
+        it("serves the HMR upgrade locally and does not forward it to the proxy", async () => {
+          const { opened, forwarded } = await probe(hmrPath);
 
-      await new Promise((resolve) => {
-        setTimeout(resolve, 200);
+          expect(opened).toBe(true);
+          expect(forwarded).toBe(false);
+        });
+
+        it("forwards a non-HMR upgrade to the user proxy", async () => {
+          const { forwarded } = await probe(nonHmrPath);
+
+          expect(forwarded).toBe(true);
+        });
       });
+    }
 
-      expect(backendUpgradeCount).toBe(before + 1);
-    });
-  });
+    // `ws`-specific: the dispatch compares the path exactly the same way
+    // `WebSocketServer#shouldHandle` does, so only the configured path (query
+    // stripped) is the HMR socket; every other variant is forwarded.
+    describe("with the `ws` server, path matching is exact", () => {
+      beforeAll(() => setup({ webSocketServer: "ws" }));
 
-  describe("HMR upgrade path matching is skipped when no webSocketServer is configured", () => {
-    let server;
-    let backend;
-    let backendUpgradeCount;
+      afterAll(teardown);
 
-    beforeAll(async () => {
-      backendUpgradeCount = 0;
+      it.each([
+        ["exact path", "/ws"],
+        ["path with query string", "/ws?token=1"],
+      ])("treats %s (%s) as the HMR upgrade path", async (_label, path) => {
+        const { forwarded } = await probe(path);
 
-      backend = http.createServer();
-      new WebSocketServer({ server: backend }).on("connection", () => {
-        backendUpgradeCount += 1;
+        expect(forwarded).toBe(false);
       });
 
-      await new Promise((resolve) => {
-        backend.listen(port5, resolve);
-      });
+      it.each([
+        ["leading double slash", "//ws"],
+        ["trailing slash", "/ws/"],
+        ["uppercase", "/WS"],
+        ["mixed case", "/wS"],
+        ["percent-encoded path", "/%77%73"],
+      ])("forwards %s (%s) to the user proxy", async (_label, path) => {
+        const { forwarded } = await probe(path);
 
-      const compiler = webpack(config);
+        expect(forwarded).toBe(true);
+      });
+    });
 
-      server = new Server(
-        {
-          hot: false,
-          liveReload: false,
-          allowedHosts: "all",
-          webSocketServer: false,
-          proxy: [
-            {
-              context: "/",
-              target: `http://localhost:${port5}`,
-              ws: true,
-            },
-          ],
-          port: port3,
-        },
-        compiler,
+    // The HMR path is read from the configured `webSocketServer` options, not a
+    // hardcoded `/ws`.
+    describe("with a custom `ws` path", () => {
+      beforeAll(() =>
+        setup({
+          webSocketServer: { type: "ws", options: { path: "/custom-hmr" } },
+        }),
       );
 
-      await server.start();
-    });
+      afterAll(teardown);
 
-    afterAll(async () => {
-      backend.closeAllConnections();
-      await server.stop();
-      await new Promise((resolve) => {
-        backend.close(resolve);
+      it("treats the configured path (/custom-hmr) as the HMR upgrade path", async () => {
+        const { forwarded } = await probe("/custom-hmr");
+
+        expect(forwarded).toBe(false);
+      });
+
+      it("forwards the default path (/ws) once it is no longer the HMR path", async () => {
+        const { forwarded } = await probe("/ws");
+
+        expect(forwarded).toBe(true);
       });
     });
 
     // With no HMR server there is no socket to protect, so the filter never
     // engages and even `/ws` is forwarded to the user proxy.
-    it("forwards /ws to the user proxy because there is no HMR socket to protect", async () => {
-      const before = backendUpgradeCount;
-
-      const ws = new WebSocket(`ws://localhost:${port3}/ws`);
-      ws.on("error", () => {});
+    describe("without a webSocketServer", () => {
+      beforeAll(() =>
+        setup({ hot: false, liveReload: false, webSocketServer: false }),
+      );
 
-      await new Promise((resolve) => {
-        setTimeout(resolve, 400);
-      });
+      afterAll(teardown);
 
-      try {
-        ws.close();
-      } catch {
-        // ignore close errors on already-failed sockets
-      }
+      it("forwards /ws to the user proxy because there is no HMR socket to protect", async () => {
+        const { forwarded } = await probe("/ws");
 
-      await new Promise((resolve) => {
-        setTimeout(resolve, 200);
+        expect(forwarded).toBe(true);
       });
-
-      expect(backendUpgradeCount).toBe(before + 1);
     });
   });