fix(proxy): match the HMR upgrade path exactly like the ws server#5678
fix(proxy): match the HMR upgrade path exactly like the ws server#5678UlisesGascon wants to merge 4 commits into
Conversation
…e configured path Adds trailing slash, case variations, percent-encoded path, and leading double slash to the existing HMR/proxy isolation tests so that all URL shapes recognized as the HMR upgrade are treated consistently.
Tighten the HMR-vs-proxy upgrade dispatch so URL variants of the
configured HMR path are consistently recognized as the HMR socket:
- lowercase comparison (case-insensitive)
- decode percent-encoding before comparison
- strip trailing slashes
- collapse leading multiple slashes so //foo is treated as /foo
instead of being parsed as a scheme-relative URL
- wrap URL parsing in try/catch so a malformed req.url does not
raise an uncaught exception
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5678 +/- ##
=======================================
Coverage 83.60% 83.61%
=======================================
Files 13 13
Lines 2086 2087 +1
Branches 773 774 +1
=======================================
+ Hits 1744 1745 +1
Misses 306 306
Partials 36 36 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/cc @bjohansebas can you take a look, thanks |
| const normalizeHmrPath = (pathToNormalize) => { | ||
| try { | ||
| return decodeURIComponent(pathToNormalize) | ||
| .toLowerCase() |
There was a problem hiding this comment.
I think the routes should be case-sensitive, just like Express handles them by default.
There was a problem hiding this comment.
The comment also comes from how ws handles its internal server, and partly from how I remember Express handling routes.
| ["uppercase", "/WS"], | ||
| ["mixed case", "/wS"], | ||
| ["percent-encoded path", "/%77%73"], |
There was a problem hiding this comment.
All of these should go to the proxy if we implement case-sensitive routes.
| */ | ||
| const normalizeHmrPath = (pathToNormalize) => { | ||
| try { | ||
| return decodeURIComponent(pathToNormalize) |
There was a problem hiding this comment.
By normalizing this ourselves, how much would it affect performance? It might be worth running some benchmarks, just as a side note, since I don't remember whether decodeURIComponent is expensive performance-wise.
|
Okay, I removed the normalization/decoding step so that it follows the ws implementation and avoids any unexpected edge cases. I'm also going to add tests for SockJS since it's still used in this version. It can be removed in the next major release. |
bjohansebas
changed the title
There was a problem hiding this comment.
Pull request overview
Adds a more faithful HMR WebSocket upgrade-path check when deciding whether to dispatch an upgrade event to user-defined WebSocket proxies, aligning the proxy “skip” behavior with how the ws server matches paths.
Changes:
- Updated
Serverupgrade handling to compare the raw request target (with query stripped) against the configured HMR path. - Added a comprehensive regression test suite ensuring HMR upgrades are served locally while non-HMR upgrades are forwarded to user proxies (including
ws,sockjs, custom paths, and disabled WS server cases).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
lib/Server.js |
Adjusts HMR upgrade-path matching to avoid URL normalization and match ws path handling semantics. |
test/server/proxy-option.test.js |
Adds new tests covering HMR vs non-HMR upgrade dispatch behavior across WS server types and configurations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| describe("HMR upgrade dispatching to user proxies", () => { | ||
| let server; | ||
| let backend; | ||
| let backendUpgradeCount; | ||
|
|
||
| // 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(); | ||
| new WebSocketServer({ server: backend }).on("connection", () => { | ||
| backendUpgradeCount += 1; | ||
| }); |
| const teardown = async () => { | ||
| backend.closeAllConnections(); | ||
| await server.stop(); | ||
| await new Promise((resolve) => { | ||
| backend.close(resolve); | ||
| }); | ||
| }; |
| let opened = false; | ||
| const ws = new WebSocket(`ws://localhost:${port3}${path}`); | ||
| ws.on("open", () => { | ||
| opened = true; | ||
| }); | ||
| ws.on("error", () => {}); | ||
|
|
||
| await new Promise((resolve) => { | ||
| setTimeout(resolve, 400); | ||
| }); | ||
|
|
No description provided.