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

Respect server.host variable in hot file #25

Merged
merged 3 commits into from Jun 24, 2022
Merged

Conversation

jessarcher
Copy link
Member

This PR adds support for Vite's server.host setting when writing the address to the hot file.

Fixes #23

@hailwood
Copy link
Contributor

hailwood commented Jun 22, 2022

@jessarcher This resolves the issue if we've manually specified the host,
If we haven't though, we still end up with 127.0.0.1 while vite defaults to localhost.

Perhaps this small update would help?

const host = configHost ?? (serverAddress === '127.0.0.1' ? 'localhost' : serverAddress)

As an aside, I think we should also be setting server.hmr.host to the same value, otherwise hmr attempts to use whatever domain we loaded the site under in our browser (e.g. I access the site in dev at https://laravel.lndo.site so it's requesting `https://laravel.lndo.site/__vite_ping and attempting to open a websocket connection to wss://laravel.lndo.site:3000)

@jessarcher
Copy link
Member Author

Ahh, I see.

I worry about specifying localhost manually as it seems like that has caused issues in the past with IPv4 and IPv6, where on some systems localhost will resolve to ::1, but in your code snippet Vite will only be listening on 127.0.0.1. See here for Vite's solution. Unless we implement a similar DNS resolution, it seems safer to use the IP address unless explicitly configured to be a hostname.

In terms of the __vite_ping thing, I don't think setting server.hmr.host will fix that as Vite 2 doesn't specify any hostname when making that request, so it will always go to the web server rather than the Vite server. This has been fixed in vitejs/vite#6819 but that only made it to Vite 3.

@hailwood
Copy link
Contributor

hailwood commented Jun 22, 2022

Okay, I don't mind manually specifying localhost in my config, as long as the hot file picks that up (which is what the PR is currently doing) so good to go there.

Re the __vite_ping thing, I wonder if that's more a side effect I end up seeing if the websocket connection fails, which is why I don't see it when setting the hmr host?
Setting the hmr host definitely corrects the issue for the websocket connection heading to the wrong location though.
Again I don't mind manually specifying that in my config if you'd rather wait and see if anyone else reports it as an issue though (or is a non issue when we're using vite3) :)

Copy link
Member

@timacdonald timacdonald left a comment

Choose a reason for hiding this comment

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

I know this is part of a bigger story for @hailwood, but I like this PR in isolation. It makes sense that we respect a hard coded config when present.

@jessarcher jessarcher merged commit 30a52b2 into main Jun 24, 2022
@jessarcher jessarcher deleted the custom-server-host branch June 24, 2022 00:04
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.

url stored in hot file does not follow vite conventions
3 participants