fix: consider undefined port when checking port

[benmccann]

Description

Closes #6068

Additional context

#7282 didn't consider undefined port

---

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.

[patak-dev]

@Conduitry would you help review and test this one?

[Conduitry]

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.

[Conduitry]

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.

[aleclarson]

See comments above

[benmccann]

To clarify, the only time a new server needs to be created is when hmr.server is undefined and the ports defined by hmr.port and server.port conflict with each other. I still maintain that my suggestion is the correct behavior. :)

I'm not sure that's what the suggestion in #7318 (comment) does? I think that perhaps the !== needs to be changed to ===?

[aleclarson]

I think that perhaps the !== needs to be changed to ===?

Ah yes, agreed!

I overlooked the fact that different protocols can share a port :)
…and I assumed the original implementation was correct (but alas it's not)

[patak-dev]

I can't believe how difficult this PR ended up being 😅

To clarify, the only time a new server needs to be created is when hmr.server is undefined and the ports defined by hmr.port and server.port conflict with each other. I still maintain that my suggestion is the correct behavior. :)

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 we are in middleware mode, we create a new server for the WebSocket Server, if not we reuse Vite Dev Server:

  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
Added support for hmr.server. This lets you use your own server for HMR, independent of Vite's Dev Server.

  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
Added support for https if a new server was needed

  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 hmr.port if it was specified.

@aleclarson the problem is that if we define upfront:

const hmrPort = hmr && hmr.port || 24678

Then hmrPort will be 24678 when you have hmr but no port defined. And then a new server will be created since server.port is 3000, no?

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)

[patak-dev]

Something else, config.server.port should be always defined if we arent in middleware mode, and we should check that we are reading it properly after overwritten if the port was occupied and auto increment kicked in

[Conduitry]

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.

[Conduitry]

This is working for me now again as of 63c0282.

[patak-dev]

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.

[patak-dev]

GitHub CI looks broken again, let's wait a bit. Maybe force push if not in a while.

[benmccann]

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.

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

[patak-dev]

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.