Bugfix/603 hono node ws duplicate event listeners #605
Conversation
🦋 Changeset detectedLatest commit: 58925fd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
||
| ;(async () => { | ||
| const events = await createEvents(c) | ||
| const ws = await nodeUpgradeWebSocket(c.env.incoming) |
There was a problem hiding this comment.
What do you think about accepting { server: { incoming: request, outgoing: undefined } } as Env?
When user use env as custom value, WebSocket helper may not work.
References: honojs/hono#2645, honojs/hono#2696, honojs/hono#2812, honojs/hono#2595 (comment).
There was a problem hiding this comment.
Thank you for your review.
First, there are many theories about the implementation of { server: { incoming: request, outgoing: undefined } }, but I consider this a minimal change to fix a bug.
Secondly, this is just my subjective view.
I see env as an area to store values given by the runtime.
Rather than storing values in the env area, I think a Variables area and middleware is a more natural way of doing this.
Again, I am not familiar with HonoJS core ideas, so this is subjective.
There was a problem hiding this comment.
Let's merge this first without including Env things!
|
Looks good! Let's go with this. Thanks! |
Fixed a bug when multiple connections are created in node-ws, as pointed out in #603
Summary of changes: