-
-
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: use hmr port if specified #7282
Conversation
The documentation for that paragraph was added in #3590 and updated #5163. I'm not familiar with this, but looking at the code it looks like the server is used directly regardless of middleware mode or https?
If I understand the docs correctly, you provide an external server (not the vite dev server) to Maybe the code has changed and the docs got stale, but I'm also a bit confused of how this is used now 🤔 |
packages/vite/src/node/server/ws.ts
Outdated
@@ -28,7 +28,7 @@ export function createWebSocketServer( | |||
let httpsServer: Server | undefined = undefined | |||
|
|||
const hmr = isObject(config.server.hmr) && config.server.hmr | |||
const wsServer = (hmr && hmr.server) || server | |||
const wsServer = (hmr && hmr.server) || (!(hmr && hmr.port) && server) |
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.
Should we check for the case where hmr.port
is the same as the main server port? We should re-use the server in that case, no?
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.
updated
I think the docs are confusing, IIUC the original PR that added them wanted to show a case where using |
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.
We don't know if the specified server.port
is occupied when creating the hmr server, the port auto-increment logic didn't kick in yet. So with the last version it may happen that the hmr.port
isn't respected.
So we have two options, supposing that hmr.port
=== server.port
-
your first commit,
a. port is free, then hmr uses the port, and the normal server auto-increments
b. port is taken, then hmr fails to listen -
your last commit,
a. port is free, then hmr and normal server uses the same port
b. port is taken, then hmr and normal server uses the next port
I think that 2 is better here
Description
Closes #6068
Additional context
I don't understand what the hmr code is trying to do and it doesn't match the behavior described in the docs, which say:
But the "assigning server.hmr.server to your HTTP(S) server" part is being completely ignored and it's using the user's server all the time. What's described in the docs is kind of a funny concept because normally you create the config and then pass it to Vite to create the server, but how would you provide the server to
server.hmr.server
when it hasn't been created yet? So I'm not really clear how this is intended to behave and there's probably more that could be fixed here.In this PR, I'm just trying to fix one specific thing that's clearly broken which is that the
hmr.port
setting is currently ignored. I'll let y'all decide if you want to do more cleanup beyond that, but it's probably beyond the scope of what I would do because I think the tests rely on the current behavior and it's not super clear what else should be changedWhat is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).