Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 4 commits into
base: v2
Choose a base branch
from

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.

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
2 participants