Skip to content

fix(client): infer socket host from script location (fix: #3093)#4168

Closed
speigg wants to merge 1 commit intovitejs:mainfrom
speigg:fix-infer-socket-host
Closed

fix(client): infer socket host from script location (fix: #3093)#4168
speigg wants to merge 1 commit intovitejs:mainfrom
speigg:fix-infer-socket-host

Conversation

@speigg
Copy link

@speigg speigg commented Jul 7, 2021

Description

Currently, the socket hostname is inferred from the current page location. This PR changes the inference logic to be based on the location of the client script, and adds support for inferring the socket port in addition to the socket hostname.

Additional context

Inferring socket location from client script location is more robust than assuming the current page is hosted by the vite dev server. For example, consider a web component being developed on a vite dev server, routed through a reverse proxy, and loaded into a web page on a different origin—by inferring socket host from client script location, this scenario "just works".

In addition, port number inference should make it easier to rely on default configuration settings when switching between localhost and/or reverse proxy configurations, or reverse proxies with random port numbers (e.g. #3093) .


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.

Infer socket hostname/port from script location

Fix vitejs#3039
@Shinigami92 Shinigami92 marked this pull request as draft July 7, 2021 20:37
@speigg
Copy link
Author

speigg commented Jul 8, 2021

I'll revise this later, need to fix broken tests.

@speigg speigg closed this Jul 8, 2021
@patak-cat
Copy link
Member

I'll revise this later, need to fix broken tests.

Great, thanks for the PR. This looks really useful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants