Run & review this pull request in StackBlitz Codeflow.

The change looks good to me.

I think we add @deprecated to server.ws.
Also add a note that these two WS related event only happens on the client side.
https://vitejs.dev/guide/api-hmr.html#hot-on-event-cb:~:text=%27vite%3Aws%3Adisconnect,is%20(re%2D)established

I am not sure if and how we can implement passing down channel so plugins can know what channel triggered HMR. WebSocket listener passes down its own connection as the second argument and I thought that maybe this is how other channels could also provide a way to distinguish themselves.

It seems that it is intended to be used to send a reply.
https://vitejs.dev/guide/api-plugin.html#handlehotupdate:~:text=Then%20use%20server,%7D%2C%0A%20%20%5D%2C%0A%7D)
So maybe it's fine to just add { send() { /* send to that client */ } } to the second argument of the callback. If we want to make it more compatible with the server.ws interface, I think we need to abstract the concept of client.

@sapphi-red can you help me with failing tests? For some reason, I get port is already in use in tests 🤔 I think ssr.spec is the easiest to debug, but I still can't find why it's failing :(

The tests were failing because HMRBroadcaster::close was not awaiting for the WebSocket server close promise. I pushed a fix 👍

The tests were failing because HMRBroadcaster::close was not awaiting for the WebSocket server close promise. I pushed a fix 👍

Thank you 🙏🏻 This is now ready for review and merge

Currently to add .hot channel the server must be created first, but plugins can send data in configureServer plugin hook before it is returned from createServer (using server.hot.on('connection')), so it will be too late in SSR case unless .hot is added by the plugin and sorted to be first:

export default {
  name: 'plugin-example',
  enforce: 'pre',
  configureServer(server) {
    server.hot.addChannel(...)
  }
}

I wonder if it would be a good idea to add channels property to server.hot config instead?

import ssrChannel from './ssrChannel'
const server = await createServer({
  server: {
    hot: {
      channels: [ssrChannel] // ws is always there for now
    }
  }
})

With this, we can also ask all channels to emit connection event before hot.on('connection') is actually triggered

I wonder if it would be a good idea to add channels property to server.hot config instead?

import ssrChannel from './ssrChannel'
const server = await createServer({
  server: {
    hot: {
      channels: [ssrChannel] // ws is always there for now
    }
  }
})

If there is no need to have the flexibility to add/remove channels after the server is created, then I think this is better. It would be good to think already how the API looks if ws is removed (I was thinking if this should be called extraChannels but it doesn't sound good, or make the user pass a WsChannel and add a init(server) to channels 🤔... or we keep channels and we accept ws as a special thing, like we do with some other config options)

If there's a usecase I'm missing about exposing channels and we need to expose them, I'm leaning towards adding channels through server.hot.channels instead to avoid the race condition.

We don't need to expose them, but we need to define them before the server is created or user-land configureServer is called because that's where server.hot.on('connection') is usually located - https://vitejs.dev/guide/api-plugin.html#server-to-client

If a new channel is added after that, then it's possible that a connection event might be triggered before the new channel is added so it will never receive this data.

One of the possible solutions is to reapply all handlers to newly added channels, but we still risk that the connection event is not emitted yet:

addChannel(channel) {
  channels.push(channel)
  appliedHandlers.forEach((event, listener) => channel.on(event, listener)) 
}

const server = await createServer()
server.hot.addChannel(new ServerHMRChannel())

I do want to find a way to have it like in the example above because the alternative requires additional code to support HMR, but I don't have a good solution:

const server = await createServer({
  server: {
    hmr: {
      channels: [new ServerHMRChannel()]
    }
  }
  // or
  plugins: [
    {
      enforce: 'pre',
      configureServer(server) {
        server.hot.addChannel(new ServerHMRChannel())
      }
    }
  ]
})

Ideally, we want to have them ready by this point:

For built-in support, we will probably just add a channel right there, but libraries would have to use either server.hmr.channels config option or configureServer plugin hook.

Thanks 👍 Yeah I think either solutions work for me. If I have to pick one, I'd prefer the server.hmr.channels config though. Plugins can use the config() hook to add new channels. That way the HMR Broadcaster don't need to expose two APIs channels and addChannel.

I guess I'm thinking that if we don't expose the concept of channels first (keep it internal only), we're free to choose either one you think is best and iterate from there. So that would unblock merging this.

That way the HMR Broadcaster don't need to expose two APIs channels and addChannel.

Another thing is that we still need to expose ws property on the server and it won't be possible if we remove .addChannel API unless you want to inject it into server.hmr.channels and find it from there when creating a server.

I guess I'm thinking that if we don't expose the concept of channels first (keep it internal only), we're free to choose either one you think is best and iterate from there. So that would unblock merging this.

channels API on the broadcaser is read-only, I am exposing it so I can later have access to it to hook into the SSR channel when creating a runtime (HMR update requires the knowledge of a runtime which can be created at any point in time). Manually pushing a new channel has no effect there. Pseudo code:

// this part is done inside "createServer" function
class ServerHMRChannel {
  name = 'ssr'
  outerEmitter = new EventEmitter()
  innerEmitter = new EventEmitter()

  send(payload) {
    this.outerEmitter.emit('send', payload)
  }

  on(event, listener) {
    this.innerEmitter.on(event, listener)
  }
}
// this is done right after websocket channel is created
const hmr = new ServerHMRChannel()
server.hot.addChannel(hmr)

// this part is a separate function and it requires an already created server
const hmr = server.hot.channels.find(c => c.name === 'ssr')!
const runtime = createViteRuntime({
  send(payload) {
    hmr.innerEmitter.emit('send', payload)
  },
  onUpdate() {
    hmr.outerEmitter.on('send', (payload) => handleHMRUpdate(payload))
  }
})

Is there a plugin using server.hmr.channels so I can see how it's being used? I still don't quite understand it after reading this thread.

@aleclarson I'm not sure if there's any. This is related to the environment API (#16358).