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): hmr websocket failure for custom middleware mode with server.hmr.server #11487

Merged
merged 3 commits into from
Jan 18, 2023

Conversation

naruaway
Copy link
Contributor

@naruaway naruaway commented Dec 26, 2022

Hello Vite team, thanks for maintaining this nice tool! I just found a bug so I would like to fix it

Description

When server.middlewareMode: true and appType: "custom", we can pass Node.js server instance to server.hmr.server option as well to make sure we use single port to serve everything (HTML, client script, and HMR WebSocket).

In this case, currently the HMR WebSocket client tries to connect to the hard-coded port (24678), which is not listened on by anything and simply causes connection error.
Please try a repro here to reproduce the bug. (UPDATE: Now I included new playground & test case in this PR so you can use them instead as a repro)

In this PR, I propose to make sure to use importMetaUrl.port under this configuration so that the HMR WebSocket client can always connect to the server reliably in any case even under HTTP reverse proxy with WebSocket support, also under a firewall to allow only a single port (e.g. 8080 port for everything)

I now also added playground/custom-server-single-port to easily confirm the fixed behavior with a test case to verify established WebSocket connection with the correct target URL.

About the importance of this bug, this setup is quite useful when the dev server can be mounted in different environments including reverse proxies (assuming the reverse proxy can proxy WebSocket as well). Making sure multiple ports works fine with any environment is hard considering there can be many intermediate things including firewall / proxy config.

Additional context

I have several things I need help from reviewers.

  • Is the logic correct? I believe this is at least an improvement but not sure about all the edge cases
  • Is the added playground / test case reasonable? Is it fine to create the whole new playground for this specific case? (I think this case is important one though)

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

Notes on the added test case in this PR

I confirmed that this added test case fails as expected on the current main branch (13ac37d) and it passes with this PR.
Here is the local test run log:

On main (13ac37d)

Test run output
$ pnpm run test-serve custom-server-single-port.spec.ts

> @vitejs/vite-monorepo@ test-serve /Users/naru/github.com/vitejs/vite
> vitest run -c vitest.config.e2e.ts "custom-server-single-port.spec.ts"


 RUN  v0.25.7 /Users/naru/github.com/vitejs/vite

x

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Failed Tests 1 ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

 FAIL  playground/custom-server-single-port/__tests__/custom-server-single-port.spec.ts > HMR WebSocket connection should use the same host as the main server
Error: net::ERR_CONNECTION_REFUSED (http://localhost:24678/)
 ❯ Page.<anonymous> playground/custom-server-single-port/__tests__/custom-server-single-port.spec.ts:10:14
      8|   new Promise<{ webSocketUrl: string; payload: any }>((resolve, reject) => {
      9|     page.on('requestfailed', (request) => {
     10|       reject(new Error(request.failure().errorText + ` (${request.url()})`))
       |              ^
     11|     })
     12|
 ❯ Page.emit node:events:513:28
 ❯ BrowserContext._onRequestFailed node_modules/.pnpm/playwright-core@1.28.1/node_modules/playwright-core/lib/client/browserContext.js:146:20
 ❯ Proxy.<anonymous> node_modules/.pnpm/playwright-core@1.28.1/node_modules/playwright-core/lib/client/browserContext.js:112:16
 ❯ Proxy.emit node:events:513:28
 ❯ Connection.dispatch node_modules/.pnpm/playwright-core@1.28.1/node_modules/playwright-core/lib/client/connection.js:178:21
 ❯ Proxy.<anonymous> node_modules/.pnpm/playwright-core@1.28.1/node_modules/playwright-core/lib/client/browserType.js:176:22
 ❯ Proxy.emit node:events:513:28
 ❯ Connection.dispatch node_modules/.pnpm/playwright-core@1.28.1/node_modules/playwright-core/lib/client/connection.js:178:21
 ❯ Immediate.<anonymous> node_modules/.pnpm/playwright-core@1.28.1/node_modules/playwright-core/lib/inProcessFactory.js:46:83

This PR

Test run output
$ pnpm run test-serve custom-server-single-port.spec.ts

> @vitejs/vite-monorepo@ test-serve /Users/naru/github.com/vitejs/vite
> vitest run -c vitest.config.e2e.ts "custom-server-single-port.spec.ts"


 RUN  v0.25.7 /Users/naru/github.com/vitejs/vite

·

 Test Files  1 passed (1)
      Tests  1 passed (1)
   Start at  21:29:56
   Duration  1.21s (transform 283ms, setup 135ms, collect 33ms, tests 418ms)

@naruaway naruaway force-pushed the fix-hmr-port-for-specified-hmr-server branch from 2d4a5d9 to b3bd80a Compare December 26, 2022 10:34
@naruaway naruaway changed the title fix(hmr): importMetaUrl.port should be used when HMR server is provided fix(hmr): HMR WS target inference should work with provided HMR server Dec 26, 2022
@naruaway naruaway force-pushed the fix-hmr-port-for-specified-hmr-server branch from a46895c to 94ef1cd Compare December 26, 2022 12:33
@naruaway naruaway changed the title fix(hmr): HMR WS target inference should work with provided HMR server fix(hmr): hmr ws target inference should work with provided hmr server Dec 26, 2022
@naruaway naruaway changed the title fix(hmr): hmr ws target inference should work with provided hmr server fix(hmr): custom middleware mode with server.hmr.server config causes HMR WebSocket failure Dec 26, 2022
@naruaway naruaway force-pushed the fix-hmr-port-for-specified-hmr-server branch from 94ef1cd to 1911e03 Compare December 26, 2022 13:24
@naruaway naruaway changed the title fix(hmr): custom middleware mode with server.hmr.server config causes HMR WebSocket failure fix(hmr): custom middleware mode with server.hmr.server causes HMR WebSocket failure Dec 26, 2022
@naruaway naruaway changed the title fix(hmr): custom middleware mode with server.hmr.server causes HMR WebSocket failure fix(hmr): hmr websocket failure for custom middleware mode with server.hmr.server Dec 29, 2022
@sapphi-red
Copy link
Member

Thanks for the PR!

I guess this will break in the case below.

  • server.hmr.server is different from the server using vite as a middleware
  • server.hmr.server listens to 24678

https://stackblitz.com/edit/github-yckrb2?file=src%2Fmain.ts

That said, I think this is a reasonable change as we suggest using server.hmr.server to use a single port.
https://vitejs.dev/config/server-options.html#server-hmr:~:text=When%20server.hmr,a%20single%20port.


About the tests, we have setup related tests here.
It would be great if you moved these test there.

These files might be helpful for writing tests there.

@sapphi-red
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Jan 1, 2023

📝 Ran ecosystem CI: Open

suite result
astro ✅ success
histoire ✅ success
iles ✅ success
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt-framework ✅ success
previewjs ✅ success
qwik ✅ success
rakkas ✅ success
sveltekit ✅ success
vite-plugin-ssr ✅ success
vite-plugin-react ✅ success
vite-plugin-react-pages ✅ success
vite-plugin-react-swc ✅ success
vite-plugin-svelte ✅ success
vite-plugin-vue ✅ success
vite-setup-catalogue ✅ success
vitepress ✅ success
vitest ✅ success
windicss ✅ success

@naruaway
Copy link
Contributor Author

naruaway commented Jan 10, 2023

@sapphi-red, thanks for great suggestions!
After some thought now I think it's better to update the current middleware-mode example in vite-setup-catalogue. I really love this setup for various reasons. Could you check sapphi-red/vite-setup-catalogue#16?
Now I also removed playground related changes from this Vite PR in favor of the vite-setup-catalogue PR

Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Quoting for visibility to other reviewers:

I guess this will break in the case ... That said, I think this is a reasonable change
#11487 (comment)

@patak-dev patak-dev merged commit 00919bb into vitejs:main Jan 18, 2023
futurGH pushed a commit to futurGH/vite that referenced this pull request Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: hmr p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants