Skip to content

Fix HMR server when WebSocket proxy is running on the same port#9432

Open
bminer wants to merge 7 commits intoparcel-bundler:v2from
bminer:v2
Open

Fix HMR server when WebSocket proxy is running on the same port#9432
bminer wants to merge 7 commits intoparcel-bundler:v2from
bminer:v2

Conversation

@bminer
Copy link

@bminer bminer commented Dec 12, 2023

↪️ Pull Request

HMR server's WebSocket always upgrades the HTTP server to a WebSocket regardless of the request URL. This patch ensures that the path must start with the HMR_ENDPOINT before doing so.

Fixes #6994

💻 Examples

None

🚨 Test instructions

Please see issue #6994. Also, this PR needs to be tested before it is merged.

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

@mischnic
Copy link
Member

Also, this PR needs to be tested before it is merged

Why the strong emphasis 😄 ? Any particular cases where you're not sure whether they work?

Would it be feasible to add an integration test (that your own WS connections can be proxied)?

describe('proxy', function () {

@mischnic
Copy link
Member

mischnic commented Jan 3, 2024

In case you didn't notice, 27 existing tests are also failing. e.g.

  1) hmr
       hmr server
         should emit an HMR update for the file that changed:
     Error: Timeout of 50000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/parcel/parcel/packages/core/integration-tests/test/hmr.js)
      at listOnTimeout (node:internal/timers:569:17)
      at processTimers (node:internal/timers:512:7)

  2) hmr
       hmr server
         should emit an HMR update for all new dependencies along with the changed file:
     Error: Timeout of 50000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/parcel/parcel/packages/core/integration-tests/test/hmr.js)
      at listOnTimeout (node:internal/timers:569:17)
      at processTimers (node:internal/timers:512:7)

@mischnic mischnic added the 📝 WIP Work In Progress label Jan 3, 2024
@bminer
Copy link
Author

bminer commented Jan 4, 2024

@mischnic - I added the strong emphasis because I committed without testing, and as a programmer, I'm sure you know that nothing ever works the first time. Haha!

Anyway, could you give me the 30-second crash course into running the test suite? I should have some time this week to get this PR working.

@mischnic
Copy link
Member

mischnic commented Jan 4, 2024

esbuild and Flow also fail, so you need these commands to see all the CI failures locally:

yarn lint
yarn flow
yarn test:integration # run all tests
yarn test:integration test/hmr.js # to run just packages/core/integration-tests/test/hmr.js

And in case you haven't done that already, before the above, run this to install deps and compile the Rust parts

yarn && yarn build-native

@bminer
Copy link
Author

bminer commented Jan 4, 2024

Okay, code is at least working now. Linters and relevant tests passing.

The primary issue here was that I forgot to modify the HMR runtime to connect to HMR_ENDPOINT instead of / to allow the HMRServer to distinguish these requests from other requests to be proxied.

Only thing left to do is actually write a test that proves #6994 is now solved.

@bminer
Copy link
Author

bminer commented Jan 4, 2024

Should be now ready to merge. Thanks for guiding me on how to run the tests. Made development a lot easier.

@bminer
Copy link
Author

bminer commented May 27, 2024

Any thoughts on when this can get merged?

mischnic added 3 commits June 2, 2024 17:02
# Conflicts:
#	packages/core/integration-tests/test/proxy.js
@GeorchW
Copy link

GeorchW commented Jul 17, 2024

Is this something that has to wait until (or is incompatible with) Parcel v3?

@bminer
Copy link
Author

bminer commented Jul 17, 2024

@GeorchW - there is no reason to wait for Parcel v3. This is a simple bug fix as far as I'm concerned.

@bminer
Copy link
Author

bminer commented Dec 27, 2024

Any reason why this has not been fixed yet?

@bminer
Copy link
Author

bminer commented Jan 20, 2025

This is still a bug with a PR that provides a fix, complete with regression tests.

Is there a reason why it has not been merged? What can I do to help?

@sebbdk
Copy link

sebbdk commented Feb 22, 2026

It's 2026 now :)

If the tests pass, could this be merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

📝 WIP Work In Progress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

WebSocket proxy fails if HMR is running on same port

4 participants