-
-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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: consider undefined port when checking port #7318
Conversation
@Conduitry would you help review and test this one? |
This looks good to me so far. I tried it out with the Vite Svelte template I mentioned in #6068, and the HMR connection to the custom port was able to connect. I'll try it out with the full SvelteKit repro as well. |
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.
This looks good to me - it works both the the Vite Svelte template and in a new SvelteKit app.
I do second @dominikg's request for some sort of comments about what all the logic here is intended to do - it looks very confusing.
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.
See comments above
This reverts commit aa1fc80.
I'm not sure that's what the suggestion in #7318 (comment) does? I think that perhaps the |
Ah yes, agreed! I overlooked the fact that different protocols can share a port :) |
I can't believe how difficult this PR ended up being 😅
This! But also you need to create it when in middleware mode so the Vite Dev server is undefined. Backtracking a bit... Code after benmccann@960b420 if (server) {
wss = new WebSocket.Server({ noServer: true })
server.on('upgrade', (req, socket, head) => {
if (req.headers['sec-websocket-protocol'] === HMR_HEADER) {
wss.handleUpgrade(req, socket, head, (ws) => {
wss.emit('connection', ws, req)
})
}
})
} else {
// vite dev server in middleware mode
wss = new WebSocket.Server({
port:
(typeof config.server.hmr === 'object' && config.server.hmr.port) ||
24678
})
} Code after #2338 const hmr = typeof config.server.hmr === 'object' && config.server.hmr
const wsServer = (hmr && hmr.server) || server
if (wsServer) {
wss = new WebSocket.Server({ noServer: true })
wsServer.on('upgrade', (req, socket, head) => {
if (req.headers['sec-websocket-protocol'] === HMR_HEADER) {
wss.handleUpgrade(req, socket, head, (ws) => {
wss.emit('connection', ws, req)
})
}
})
} else {
// vite dev server in middleware mode
wss = new WebSocket.Server({
port: (hmr && hmr.port) || 24678
})
} Code after #1992 const hmr = isObject(config.server.hmr) && config.server.hmr
const wsServer = (hmr && hmr.server) || server
if (wsServer) {
wss = new WebSocket({ noServer: true })
wsServer.on('upgrade', (req, socket, head) => {
if (req.headers['sec-websocket-protocol'] === HMR_HEADER) {
wss.handleUpgrade(req, socket as Socket, head, (ws) => {
wss.emit('connection', ws, req)
})
}
})
} else {
const websocketServerOptions: ServerOptions = {}
const port = (hmr && hmr.port) || 24678
if (httpsOptions) {
// if we're serving the middlewares over https, the ws library doesn't support automatically creating an https server, so we need to do it ourselves
// create an inline https server and mount the websocket server to it
httpsServer = createHttpsServer(httpsOptions, (req, res) => {
const statusCode = 426
const body = STATUS_CODES[statusCode]
if (!body)
throw new Error(
`No body text found for the ${statusCode} status code`
)
res.writeHead(statusCode, {
'Content-Length': body.length,
'Content-Type': 'text/plain'
})
res.end(body)
})
httpsServer.listen(port)
websocketServerOptions.server = httpsServer
} else {
// we don't need to serve over https, just let ws handle its own server
websocketServerOptions.port = port
}
// vite dev server in middleware mode
wss = new WebSocket(websocketServerOptions)
} And this is where #7282 tried to fix respecting @aleclarson the problem is that if we define upfront: const hmrPort = hmr && hmr.port || 24678 Then hmrPort will be 24678 when you have Aren't we looking for something like? const hmr = isObject(config.server.hmr) && config.server.hmr
const hmrServer = hmr && hmr.server
const hmrPort = hmr && hmr.port
const portsAreCompatible = !hmrPort || !config.server.port || hmrPort === config.server.port
const wsServer = hmrServer || (portsAreCompatible && server) |
Something else, |
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.
The latest version (as of 64f2d07) doesn't seem to be working for me. The browser attempts to connect to the custom port, but it can't make the connection.
When the code gets to
vite/packages/vite/src/node/server/ws.ts
Line 37 in 64f2d07
if (wsServer) { |
wsServer
is truthy, which I don't think is correct. We need it to be falsy so that the separate WS server is created in the other branch.
This is happening because portsAreCompatible
is true
, which is happening because config.server.port
is undefined.
This is working for me now again as of 63c0282. |
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.
The only issue that I see with this is that if the user sets hmr.port: 3000
, then it will create a new server for HMR, and the regular dev server will end up at 3001
. So it isn't ideal. But I think this should be rare, and it is better than what we currently have so let's merge it.
GitHub CI looks broken again, let's wait a bit. Maybe force push if not in a while. |
Thanks for the pragmatic decision on this. That's basically my view of it as well. We could add a TODO or something if you'd like |
Not a bad idea, a short TODO comment, or if not you could create an issue so we can keep track of this. About GitHub... things are broken https://www.githubstatus.com/, again. So we just wait. |
Description
Closes #6068
Additional context
#7282 didn't consider undefined port
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).