-
-
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 client port from page location when using default port #7977
Conversation
As mentioned in #7463 (comment), I could also imagine to infer the port when
|
@Artur- @baileyherbert @lbogdan I would love your feedback on this. |
I have not given this much thought, just got involved because the earlier PR broke our setup. To me it sounds like unnecessarily much magic though to handle default ports separately. For our setup, I have no issue in defining configuration options to make it use the correct port. I would prefer not to specify the port though, just a boolean option for "connect directly to the Vite server, bypassing any possible proxy" |
Nevermind my review, I misunderstood the intent here. This looks good! 👍🎉 I tested on both a custom local hostname and with an Argo Tunnel – it's all working as expected. Inferring the port automatically when using a custom local hostname would be nice, but isn't nearly as important. This pull request is sufficient for my use cases. Cheers! |
Would this solve the issue where one has a vite dev server up on for example port 5000, and a reverse proxy on port 6000 that proxies to port 5000? I have observed that the vite hmr client does still try to stubbornly go to port 5000 in such a scenario presumably because it gets the destination origin instead of just using the current origin. |
@lukashass would you explain what would be the configuration needed in these cases to make these breaking changes work after the PR? |
This PR currently only automatically infers the port when your reverse proxy exposes a default port (80 or 443). In your case you would still need to set |
@patak-dev Both cases can be solved by explicitly setting |
@lbogdan would you review this approach to check it works well for your needs? @lukashass about the effect of this PR when there isn't a proxy, IIUC, this doesn't change things in that case, no? |
Just tested this on my own project and it works -- it would allow us to get rid of some rather obtuse config along the lines of... const hmrPort = process.env.HMR_PORT
? Number.parseInt(process.env.HMR_PORT, 10)
: undefined; hmr: { clientPort: hmrPort ?? port }, We have many "development targets" with their own prewritten sets of env variables -- right now, when running through a proxy we have to explicitly set This PR also seems like the "right" behavior for proxies generally -- if the default assumption is to expect HMR to work on the same domain/port combo that the app is being accessed over, I would expect Vite to adjust to the new domain/port of the proxy. I can't think of any situations off the top of my head where this wouldn't be the right behavior (though I imagine there are a few). |
I tried to organize all the usage. browser loc: page origin opened with browser A: Vite server listening origin (Vite server knows this)
Does this cover everything? (It's very complicating...) If that is true, I have a suggestion. (Sorry for not suggesting until now.) If we default hmr to same port with server (I'm not sure if this is possible), I think HMR WebSocket connection target could be inferred like:
This will work for all cases if I'm not missing something. |
IIUC this PR will break backend integration setup when the backend server is running on default port. It could be solved by setting |
Looks to be working now with vite 3.0, the dev websocket connects to the correct port behind a reverse proxy. |
Description
Closes #5153.
By inferring the HMR client port from the current page location when accessing vite via a default port, vite can be run behind reverse proxies without extra configuration.
Additional context
This is a continuation of the discussion in #7463. In contrast to #7463 (comment), I decided against changing the
clientPort
type for now as it is not entirely necessary.This will be a breaking change when you are:
hmr.port
is setWhat is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).