WebSocket hooks don't fire when using the exported `listen` function from Listhen

[joshmossas]

Environment

NodeJS - v20.11.1 H3 - v1.11.1

Reproduction

https://github.com/joshmossas/h3-ws-listhen-bug

Describe the bug

If you start an h3 server like so:

listen(toNodeListener(app), {
  ws: true,
  public: true,
});

None of the websocket hooks in defineWebSocketHandler will fire. Oddly enough, clients are still able to connect and send messages. So listhen is upgrading the connection, but for some reason none of the hooks go off meaning you can't actually send messages back.

If you refactor the code to use the listhen cli. It starts working again:

// index.ts
export default app;

listhen ./index.ts --ws --public

Additional context

I'm unsure if the bug is happening on the H3 side or the Listhen side. I did try digging through both code bases for a while and couldn't figure out what was causing this behavior.

Logs

No response

[joshmossas]

Okay I've determined the issue. It turns out the listhen cli does some magic to assign the websocket hooks from the h3 app handler. The raw listen function doesn't do any of that magic so you have to tell it how to resolve the websocket hooks manually.

To get things working I had to change it from

listen(toNodeListener(app), {
  ws: true,
  public: true,
});

to the following

listen(toNodeListener(app), {
  ws: {
    async resolve(info) {
      if (app.websocket.resolve) {
        return app.websocket.resolve(info);
      }
      return app.websocket.hooks ?? app.handler.__websocket__ ?? {};
    },
  },
  public: true,
});

Perhaps there should be some documentation added regarding this? Or is the goal that the listhen listen function also handles this automatically out of the box?

[manniL]

This seems more like a listhen issue then.

It is also denoted in the README that the CLI does enable it easily

[joshmossas]

Hmm, so then I think the main thing that needs to be fixed is updating the exported ListenOptions type

Right now ws accepts boolean, CrossWSOptions, or ((req: IncomingMessage, head: Buffer) => void), but using a boolean doesn't actually work. So the typing is deceptive.

It looks like the main reason why the type allows for a boolean is because the CLI reuses the ListenOptions type so we would need to some slight modifications to the typing in the CLI to make it work. From an initial glance the cli could prolly use something like this

Omit<ListenOptions, "ws"> & { ws?: boolean | CrossWsOptions | ((req: IncomingMessage, head: Buffer) => void) }

(This is pseudo code I would need to actually test in an editor haha)

I'd be willing to open a PR for this if the team is open to my proposed (pseudo code) solution. Fixing this would remove future confusion surrounding this feature imo, especially since the README does outline the proper usage. Although a more detailed example probably wouldn't hurt either.

[pi0]

Thanks for investigations and initial reproduction. I think we can do as you proposed in https://github.com/unjs/listhen/issues/174#issuecomment-2094799544 also in programmatic when true value is passed.

[joshmossas]

Okay sure. That would create a more seamless dev experience!

I suppose the only blocker for what I wrote in https://github.com/unjs/listhen/issues/174#issuecomment-2094799544 is that it has access to the original H3 app giving me access to app.websocket.whatever. The listen() function would only have access to the result of toNodeListener(). Is there a way to get the original H3 app out of a NodeListener?

If not we can take a couple approaches:

1. Have H3 modify the NodeListener by adding a webhook resolver that listhen can check for. This will make ws: true work but only for H3 apps.
2. Make use of logic similar to what the CLI does. (It looks like it creates a parent H3 app which wraps the NodeListener and use that to resolve webhooks).

The second option is prolly more self contained. I can start exploring that unless you think the first option is preferable.

[pi0]

Exploring with second is good start. We might find a lighter wrapper to replace h3 layer in both places