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

The "event" argument must be an instance of Event. Received an instance of Event (WebSocket + ws) #2663

Closed
kettanaito opened this issue Jan 29, 2024 · 19 comments
Labels
bug Something isn't working

Comments

@kettanaito
Copy link
Contributor

kettanaito commented Jan 29, 2024

Bug Description

When Undici calls fireEvent with the MessageEvent received from the server constructed via the ws package, the following error is thrown by internals/event_target:

The "event" argument must be an instance of Event. Received an instance of Event

Reproducible By

import { WebSocketServer } from 'ws'
import { WebSocket } from 'undici'

const server = new WebSocketServer({ host: '127.0.0.1', port: 0 })
server.on('connection', (client) => {
  client.on('message', (data) => {
    client.send('greetings')
  })
})

const ws = new WebSocket(server.address())
ws.addEventListener('open', () => {
  ws.send('hello')
})
ws.addEventListener('message', (event) => {
  ws.send('client received:', event.data)
})

Expected Behavior

The MessageEvent dispatched via fireEvent does not throw an error.

Logs & Screenshots

Screenshot 2024-01-29 at 14 25 25

Environment

os: MacOS 14.2.1 (23C71)
node: v18.17.0

Additional context

Here's the check that Node performs before throwing that error:

https://github.com/nodejs/node/blob/64c6d97463c29bade4d6081683dab2cd7cda298d/lib/internal/event_target.js#L751

I suspect that the MessageEvent provided by the ws package doesn't extend the global Event correctly, and thus fails the instanceof check. I still think it's worth opening the issue in Undici for discoverability even if the root cause will end up to be elsewhere.

@kettanaito
Copy link
Contributor Author

Closing because it's the issue in the ws package (see websockets/ws#1818 (comment)).

I suspect that due to the Node.js version support they promise, they ended up re-implementing Event and EventTarget, which leads to the MessageEvent extends Event extending the wrong Event (their own, not the global one; see https://github.com/websockets/ws/blob/5e42cfdc5fa114659908eaad4d9ead7d5051d740/lib/event-target.js#L17).

@kettanaito
Copy link
Contributor Author

Strangely, even if I modify ws to dispatch a global Event, its instanceof check still returns false, even if inlined in Undici's fireEvent. Can the two packages be getting different global Event object? 🤔

@lpinca
Copy link
Member

lpinca commented Jan 29, 2024

I don't understand. In the example ws does not fire any event. The event is fired by undici. What ws sends to undici is just a WebSocket frame.

@kettanaito
Copy link
Contributor Author

kettanaito commented Jan 29, 2024

@lpinca ws sends an Event instance (i.e. the new Event('open')), which undici then provides to fireEvent, triggers dispatchEvent of the Node's native EventTarget, and the error is thrown. Perhaps I should've stressed that this is what happens under the hood.

@kettanaito
Copy link
Contributor Author

kettanaito commented Jan 29, 2024

Here's a detailed call stack:

At its core, it's class identity issue (ws re-implementing Event). But as I've stated above, even if I patch ws and drop event-target.js from it entirely, the events that arrive at Undici still fail the instanceof check. Most likely me patching things wrong.

@lpinca
Copy link
Member

lpinca commented Jan 29, 2024

There is no listener for the 'open' event in the example.

@kettanaito
Copy link
Contributor Author

kettanaito commented Jan 29, 2024

@lpinca there's a listener for the message event though. The same thing happens to all events from ws (open, message, error, close) since none of them extend the global Event in Node. The ws module cannot be used with the WebSocket class from undici at its current state.

@lpinca
Copy link
Member

lpinca commented Jan 29, 2024

But you are adding it to the the undici WebSocket instance, not ws WebSocket instance. You are not using ws event-target.js at all in the example.

@kettanaito
Copy link
Contributor Author

I apologize, I extracted my issue scenario incorrectly 👍 Will update to prevent further confusion.

@ronag
Copy link
Member

ronag commented Jan 29, 2024

I don't see how this is related to ws. If there is an issue it's in undici. @KhafraDev

@KhafraDev
Copy link
Member

the repro works fine for me, is there an example where it doesn't?

@lpinca
Copy link
Member

lpinca commented Jan 29, 2024

I think they are doing something like websockets/ws#1818 but I see nothing related to ws in the stack trace.

@KhafraDev
Copy link
Member

error is coming from:

function fireEvent (e, target, eventConstructor = Event, eventInitDict) {
  const event = new eventConstructor(e, eventInitDict) // eslint-disable-line new-cap
  target.dispatchEvent(event)
}

fireEvent('open', <websocket instance>)

I can only see this being an issue if Event is being overridden (a la jest). I don't think this is an undici issue either without more info.

@kettanaito
Copy link
Contributor Author

@KhafraDev, like I suspected, this isn't an issue with Undici. It simply surfaces through it.

The issue is in the ws module (see websockets/ws#1818).

@lpinca
Copy link
Member

lpinca commented Feb 8, 2024

Can you please share a minimal test case? The one in the issue description works correctly. websockets/ws#1818 has nothing to do with this unless you take a ws Event and manually dispatch it with undici's WebSocket which does not make sense to me.

@KhafraDev
Copy link
Member

Since ws' Event is similar to the spec one, it should be possible to take the attributes from ws' Event and create a node Event from that.

@kettanaito
Copy link
Contributor Author

kettanaito commented Feb 9, 2024

Reproduction case

  1. Check out feat: add WebSocket class interceptor mswjs/interceptors#501.
  2. Open test/modules/WebSocket/exchange/websocket.server.connect.test.ts. Uncomment any/all server tests.
  3. pnpm test test/modules/WebSocket/exchange/websocket.server.connect.test.ts

See the following errors during the test run:

TypeError: The "event" argument must be an instance of Event. Received an instance of Event
 ❯ new NodeError ../node:internal/errors:405:5
 ❯ WebSocket.dispatchEvent ../node:internal/event_target:678:13
 ❯ fireEvent ../node_modules/.pnpm/[email protected]/node_modules/undici/lib/websocket/util.js:58:10
 ❯ WebSocket.#onConnectionEstablished ../node_modules/.pnpm/[email protected]/node_modules/undici/lib/websocket/websocket.js:537:5
 ❯ ../node_modules/.pnpm/[email protected]/node_modules/undici/lib/websocket/websocket.js:126:50
 ❯ Object.processResponse ../node_modules/.pnpm/[email protected]/node_modules/undici/lib/websocket/connection.js:197:7
 ❯ ../node_modules/.pnpm/[email protected]/node_modules/undici/lib/fetch/index.js:1083:38
 ❯ ../node:internal/process/task_queues:140:7
 ❯ AsyncResource.runInAsyncScope ../node:async_hooks:203:9
 ❯ AsyncResource.runMicrotask ../node:internal/process/task_queues:137:8

This reproduction case is a part of the Interceptors library. Note that the interceptor itself doesn't affect Event, Undici, or ws in any way. I can put up a more isolated repository if you wish but the instructions above will give you the errors I'm experiencing.

The test in question will produce the same error even if you remove the interceptor part from it, which leads me to believe something else is the root cause here.

More context

I learned that Undici also implements its own events but they do extend the global Event class, which should make individual events correct instances of the global Event.

However, that's not the case. Here's when Undici emits its own MessageEvent:

// 3. Fire an event named message at the WebSocket object, using MessageEvent,
// with the origin attribute initialized to the serialization of the WebSocket
// object’s url's origin, and the data attribute initialized to dataForEvent.
fireEvent('message', ws, MessageEvent, {
origin: ws[kWebSocketURL].origin,
data: dataForEvent
})

If I check event instanceof Event in fireEvent(), I will get false:

const event = new eventConstructor(e, eventInitDict) // eslint-disable-line new-cap

console.log(event instanceof Event)
false

I'm a bit confused why this is happening even if it's my specific issue. The only case I can think of when this would happen is when the global Error points to different classes in Undici and in Node.js. Undici doesn't do anything to result in this. The Interceptors library also doesn't do anything to result in this. Odd.

This instance check later on results in the Node.js error during the event validation here:

https://github.com/nodejs/node/blob/8a41d9b636be86350cd32847c3f89d327c4f6ff7/lib/internal/event_target.js#L751

I would be grateful if you point me in the right direction and explain why is that instance check failing.

@kettanaito
Copy link
Contributor Author

This is likely an environment problem. Standalone, Undici events pass the instanceof check:

const { MessageEvent } = require('undici/lib/websocket/events.js')
console.log(new MessageEvent('message') instanceof Event)
// true

I will try to see if this has anything to do with the test runner I'm using.

@kettanaito
Copy link
Contributor Author

Root cause

This error was happening due to JSDOM. It's always JSDOM. Looks like the global Event integrity is violated when you use Jest+JSDOM. This has cost me almost a week of debugging.

Thank you everyone for help here. I appreciate it a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants