Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
JeromeDeLeon opened this issue Oct 19, 2024 · 3 comments
Open

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

JeromeDeLeon opened this issue Oct 19, 2024 · 3 comments

Comments

@JeromeDeLeon
Copy link

JeromeDeLeon commented Oct 19, 2024

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
Copy link
Member

@JeromeDeLeon Thank you for the issue.

@nakasyou Can you take a look?

@nakasyou
Copy link
Contributor

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
Copy link
Author

JeromeDeLeon commented Oct 20, 2024

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
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants