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: infer client port from page location when using default port #7977

Closed
wants to merge 2 commits into from

Conversation

lukashass
Copy link
Contributor

@lukashass lukashass commented May 1, 2022

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:

  • accessing your application through a proxy that listens on a default port (80, 443) and does not support websockets
  • accessing your application through a proxy that listens on a default port (80, 443) and hmr.port is set

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 Commit 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.

@sapphi-red sapphi-red added feat: hmr p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) labels May 1, 2022
@lukashass
Copy link
Contributor Author

As mentioned in #7463 (comment), I could also imagine to infer the port when location.hostname !== 'localhost'. This would also solve #3093, but introduce a breaking change when you are:

  • accessing vite through a proxy that listens on a hostname that is not localhost and does not support websockets

@lukashass
Copy link
Contributor Author

@Artur- @baileyherbert @lbogdan I would love your feedback on this.

@patak-dev patak-dev added this to the 3.0 milestone May 2, 2022
@Artur-
Copy link
Contributor

Artur- commented May 2, 2022

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"

@baileyherbert
Copy link

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!

@silverwind
Copy link

silverwind commented May 12, 2022

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.

@patak-dev
Copy link
Member

This will be a breaking change when you are:

  • accessing vite through a proxy that listens on a default port (80, 443) and does not support websockets
  • accessing vite through a proxy that listens on a default port (80, 443) and hmr.port is set

@lukashass would you explain what would be the configuration needed in these cases to make these breaking changes work after the PR?

@lukashass
Copy link
Contributor Author

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.

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 clientPort: 6000.

@lukashass
Copy link
Contributor Author

@lukashass would you explain what would be the configuration needed in these cases to make these breaking changes work after the PR?

@patak-dev Both cases can be solved by explicitly setting clientPort to the actual vite port i.e. circumventing the proxy for the vite websocket.

@patak-dev
Copy link
Member

@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?

@davidwallacejackson
Copy link
Contributor

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 HMR_PORT=443. This has the unfortunate side-effect of making the app continuously reload if you try to bypass the proxy (because it fails to connect to HMR and does a full reload). Bypassing the proxy is useful sometimes for debugging purposes, so this is inconvenient.

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).

@sapphi-red
Copy link
Member

sapphi-red commented Jun 16, 2022

I tried to organize all the usage.

browser loc: page origin opened with browser
browser js: JS request (cf. <script type="module" src="/@vite/client">) target origin from browser
browser hmr: HMR WebSocket connection target origin from browser
vite server: Vite server listening origin
vite hmr server: Vite hmr server listening origin

A: Vite server listening origin (Vite server knows this)
B: Vite hmr server listening origin (Vite server knows this)
C: backend server/proxy origin (Vite server does not know this)

browser loc browser js browser hmr vite server vite hmr server details
1 A A B A B normal
2 A A A A A normal (hmr same port)
3 A A B - B middleware mode (without websocket proxy)
4 A A A - B middleware mode (with websocket proxy)
5 C A B A B backend server (without websocket proxy)
6 C A A A A backend server (hmr same port)
7 C C B A B proxy before vite (without websocket proxy)
8 C C C A A proxy before vite (hmr same port)

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:

  1. if hmr.host / hmr.port is set (make them all or none), use that
  2. get origin from import.meta.url, use that

This will work for all cases if I'm not missing something.
Also this will remove the need for having hmr.clientPort, and hmr.* might be replaced with hmr.origin.

@sapphi-red
Copy link
Member

IIUC this PR will break backend integration setup when the backend server is running on default port. It could be solved by setting server.hmr.clientPort = server.port.

@sapphi-red
Copy link
Member

Closing as #8650 is merged.
Thank you all for the PR and feedbacks/reviews!
Please let us know if #8650 works for your use cases.

@sapphi-red sapphi-red closed this Jun 23, 2022
@silverwind
Copy link

Looks to be working now with vite 3.0, the dev websocket connects to the correct port behind a reverse proxy.

@lukashass lukashass deleted the improve-client-port-default branch October 6, 2022 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: hmr on hold p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

HMR client port should default to 443 on https
7 participants