honojs / middleware

monorepo for Hono third-party middleware/helpers/wrappers
https://hono.dev
478 stars 173 forks source link

[@hono/node-ws] ReferenceError: ErrorEvent is not defined #784

Open JeromeDeLeon opened 1 month ago

JeromeDeLeon commented 1 month ago

I'm using Node.js v22.10.0 with @hono/node-ws v1.0.4 and when listening to the onError event it throws the following error:

new ErrorEvent("error", {
                ^
ReferenceError: ErrorEvent is not defined

My workaround is to use undici package and import that event from there.

I'm checking if that event exists because I thought it was unavailable to Node.js - https://developer.mozilla.org/en-US/docs/Web/API/ErrorEvent#browser_compatibility.

I managed to test it via


upgradeWebSocket(() => ({
  onOpen(e) {
    console.log('onOpen', e);
  },
  onMessage(e, ctx) {
    console.log('onMessage', e);

    // simulate error event
    ctx.raw?.emit('error', new Error('Simulated error'));
  },
  onClose(e) {
    console.log('onClose', e);
  },
  onError(e) {
    console.log('onError', e);
  },
}))
yusukebe commented 1 month ago

@JeromeDeLeon Thank you for the issue.

@nakasyou Can you take a look?

nakasyou commented 1 month ago

There are two ways to close this I think.

  1. Make adapter depend undici and use it.
  2. Use Event instead of ErrorEvent.

From spec, onerror can use both of Event and ErrorEvent.

Reference:

JeromeDeLeon commented 1 month ago

Since onError event callback uses Event type, I think it makes sense to follow that as well. That way, we lessen the library to install just for ErrorEvent until Node.js directly supports it. The only issue with this is the Event type doesn't allow passing the error property to Event. Maybe we could implement a custom ErrorEvent that extends the Event class just for the error property. 🤔

While we're at it, can I also request to change the type of the NodeWebSocket to be like the diff below? This is to add type to ctx.raw since it is the ws. Right now, the type is unknown because UpgradeWebSocket defaults the generic type parameter to unknown.

The injectWebSocket change doesn't affect anything. It is more of reusing what already exists from @hono/node-server.

-import type { Server } from 'node:http'
-import type { Http2SecureServer, Http2Server } from 'node:http2'
+import { ServerType } from '@hono/node-server'

export interface NodeWebSocket {
-  upgradeWebSocket: UpgradeWebSocket
+  upgradeWebSocket: UpgradeWebSocket<WebSocket> 
-  injectWebSocket(server: Server | Http2Server | Http2SecureServer): void
+  injectWebSocket(server: ServerType): void
}