-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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: infer hmr ws target by client location #8650
fix: infer hmr ws target by client location #8650
Conversation
This is amazing @sapphi-red: https://github.com/sapphi-red/vite-setup-catalogue |
The with-proxy setup in vite-setup-catalogue requires docker so I think it is better to use vite-ecosystem-ci. (or remove docker) I need to solve how to make websocket proxy by-passed setup work without configuration. |
I think vite-ecosystem-ci would be a good place for this 👍🏼 |
I implemented the fallback feature which I said in the description of this PR: "To avoid a breaking change: If the connection failed, I think we could fallback to the Vite HMR server." |
@lukashass @lbogdan we're merging this PR because it looks like it covers the required cases, and it has been tested in several scenarios thanks to @sapphi-red's new suite. We would like to have this change in the v3 beta (to be released soon) to get proper feedback from the ecosystem. We still have time to modify the default behavior if you find issues with this approach. Please let us know if this works for your use cases. |
Released as vite@3.0.0-alpha.14 in case, so it should be easier to test 👍🏼 |
const socketHost = `${__HMR_HOSTNAME__ || location.hostname}:${__HMR_PORT__}` | ||
const hmrPort = __HMR_PORT__ | ||
const socketHost = `${__HMR_HOSTNAME__ || importMetaUrl.hostname}:${ | ||
hmrPort || importMetaUrl.port |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: I forgot to handle a case when importMetaUrl.port
is ''
. But surprisingly this actually works.
new URL("ws://localhost:").port // ''
new WebSocket("ws://localhost:") // connects to `ws://localhost`
Necessary to avoid a warning when using a fallback after the connection fails, since `rack-proxy` does not support proxying websockets. vitejs/vite#8650
I think this fix does not work with provided HMR WS server, which is important case to achieve "single port for everything". |
Description
This PR implements #7977 (comment).
(That comment says "If we default hmr to same port with server" but it was already working like that.)
I created https://github.com/sapphi-red/vite-setup-catalogue and tested all of these setups and it worked without any configuration.
(with-proxy includes a configuration because this repo uses
3.0.0-alpha.12
not this PR)fixes #3093
fixes #5153
fixes #6814
Cases covered with vite-setup-catalogue
Cases not covered with vite-setup-catalogue
Vite runs behind a reverse-proxy which does not support websocket (fix: infer client port from page location #7463 (comment), fix: infer client port from page location #7463 (comment))It is a breaking change for this setup. Previously it worked without settings. Now it needs to setserver.hmr.clientPort = server.port
orserver.hmr.port = 'something'
.To avoid a breaking change: If the connection failed, I think we could fallback to the Vite HMR server. (not yet implemented in this PR)server.hmr.port
is set (2.9.1portsAreCompatible
is a breaking change #7545)Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).