websockets / ws

Simple to use, blazing fast and thoroughly tested WebSocket client and server for Node.js
MIT License
21.68k stars 2.42k forks source link

Use Node.js 15 native `EventTarget` object #1818

Open piranna opened 3 years ago

piranna commented 3 years ago

Description

Node.js 15 already provides a native implementation of EventTarget, so there's no need to use our own implementation. In fact, using both of them at the same time leads to errors.

In my use case, I've created a Client class that extends from Node.js native EventTarget class, that internally it's using an ws instance (and doing some other project specific things), and setting as listeners methods from this Client class, with the idea of propagate these errors to the user:

export class Client extends EventTarget
{
  constructor(ws)
  {
    super()

    if(!(ws instanceof WebSocket)) ws = new WebSocket(ws)

    ws.binaryType = 'arraybuffer'

    ws.addEventListener('close', this.#onClose, {once: true})
    ws.addEventListener('error', this.#onError)
    ws.addEventListener('message', this.#onMessage)
    ws.addEventListener('open', this.#onOpen, {once: true})

    this.#ws = ws
  }

  #onClose = this.dispatchEvent.bind(this)
  #onError = this.dispatchEvent.bind(this)
  #onOpen  = this.dispatchEvent.bind(this)
}

Problem is, since ws is using its own implementation of both EventTarget and Event classes, when these events gets propagated to the native one, I get the next error:

TypeError [ERR_INVALID_ARG_TYPE]: The "event" argument must be an instance of Event. Received an instance of ErrorEvent
    at new NodeError (node:internal/errors:277:15)
    at EventTarget.dispatchEvent (node:internal/event_target:326:13)
    at WebSocket.onError (/home/piranna/Trabajo/Atos/awrtc_signaling/node_modules/ws/lib/event-target.js:141:16)
    at WebSocket.emit (node:events:329:20)
    at WebSocket.EventEmitter.emit (node:domain:467:12)
    at ClientRequest.<anonymous> (/home/piranna/Trabajo/Atos/awrtc_signaling/node_modules/ws/lib/websocket.js:579:15)
    at ClientRequest.emit (node:events:329:20)
    at ClientRequest.EventEmitter.emit (node:domain:467:12)
    at TLSSocket.socketErrorListener (node:_http_client:478:9)
    at TLSSocket.emit (node:events:329:20) {
  code: 'ERR_INVALID_ARG_TYPE'
}

This is due because Node.js native EventTarget class is expecting a native Event class instance, instead of the one provided by ws. According to Node.js docs it should be accepting any object with a type field, but for some reason is not accepting it.

Reproducible in:

Steps to reproduce:

  1. use Node.js 15
  2. create an object instance with Node.js 15 native EventTarget class in its prototype chain
  3. create a ws instance and call to addEventListener setting one function that propagate the event to the native EventTarget
  4. emit the event
  5. BOOM

Expected result:

ws should check for actual support of both Event and EventTarget classes in the native platform (in this case, Node.js 15) and use them. In case they are not available, then use its own implementation as a polyfill.

Actual result:

ws is using always its own implementation of Event and EventTarget classes, since there was none before, so now it conflicts with the new Node.js native available ones.

lpinca commented 3 years ago

I think it is not possible to use the Node.js EventTarget implementation without introducing breaking changes and performance regressions.

The ws implementation relies on the EventEmitter interface.

If I could go back in time I would have never made the WebSocket an EventTarget. Anyway the feature was added before I started contributing to ws and I think the reason was to have a browser compatibile interface.


Can't you use the EventEmitter interface in your code? Make Client inherits from EventTarget and use ws.{on,once}() instead of ws.addEventListener().

piranna commented 3 years ago

I think it is not possible to use the Node.js EventTarget implementation without introducing breaking changes and performance regressions.

Or maybe yes :-) NodeEventTarget extends from EventTarget and implements the EventEmitter API on top of it. I read in the development discussion that it was specifically designed for compatibility and migration issues, so if you are concerned about that, probably this would be the correct aproach to begin with.

Can't you use the EventEmitter interface in your code? Make Client inherits from EventTarget and use ws.{on,once}() instead of ws.addEventListener().

I didn't consider that, since by inertia I always use the W3C API for compatibility between browser and Node.js, but yes, it's something I can do since in this case it would be a Node.js only code :-)

lpinca commented 3 years ago

Or maybe yes :-) NodeEventTarget extends from EventTarget and implements the EventEmitter API on top of it. I read in the development discussion that it was specifically designed for compatibility and migration issues, so if you are concerned about that, probably this would be the correct aproach to begin with.

Ok, but NodeEventTarget currently only implements a subset of the EventEmitter API. Some important missing API are emitter.prependListener() and emitter.listeners(). It would still be a breaking change and I'm still worried about performance regressions. I didn't follow the EventTarget implementation closely in Node.js and I'm not sure if it was about NodeEventTarget but I remember some benchmarks where EventTarget was an order of magnitude slower than the EventEmitter.

piranna commented 3 years ago

One important missing API is emitter.prependListener() and emitter.listeners(). It would still be a breaking change

Is it actually being used? We could take a look for what NodeEventTarget missing features are being used and identify if they can be fixed someway.

I'm still worried about performance regressions. I didn't follow the EventTarget implementation closely in Node.js and I'm not sure if it was about NodeEventTarget but I remember some benchmarks where EventTarget was an order of magnitude slower than the EventEmitter.

I somewhat remember something about that, but an order of magnitude is too much... How are ws benchmarks being done? Maybe we can work in a separate branch and check for them...

piranna commented 3 years ago

Reading Node.js EventTarget source code, it shows events list is implemented with a private SafeMap, and that's what NodeEventTarget uses for the listenerCount() count method. We would need to change its implementation, but if both emitter.prependListener() and emitter.listeners() are needed and can be used an alternative, seems it's possible to implement them there... just not something immediate.

lpinca commented 3 years ago

I prefer to keep making WebSocket inherit from EventEmitter for now.

Advantages:

Disadvantages:

That said, I think making WebSocket inherit from NodeEventTarget is something worth exploring.

netizen-ais commented 3 years ago

Also there are some node packages with binary blobs that aren't ready, and don't yet work under Node v15. I use one of those closely tied to ws (wrtc)

piranna commented 3 years ago

After taking a more carefully look, probably this can be splitted in two tasks:

  1. use native Event class
  2. use native EventTarget or NodeEventTarget class

First one is just a data container, so it's easy to replace, and that would fix the current error since it's doing a validation that the provided object inherit from the Event class, so it would work and probably second one would not be needed (or would get a lower priority).

piranna commented 3 years ago

target field of Event class is read-only, and we are assigning it. I've reviewed Node.js source code and it's only set by private [kHybridDispatch]() method, so it's not possible to only do first step, we'll need to go directly to use Node.js native EventTarget or NodeEventTarget class, not just only the native Event class :-/

jimmywarting commented 3 years ago

Would very much also want to see EventTarget being used also

danieltroger commented 3 years ago

I just had this dumb error too:

TypeError [ERR_INVALID_ARG_TYPE]: The "event" argument must be an instance of Event. Received an instance of OpenEvent
    at new NodeError (node:internal/errors:363:5)
    at EventTarget.dispatchEvent (node:internal/event_target:401:13)
   [...]

Can you make a fork or something that uses proper EventTargets?

lpinca commented 3 years ago

With the introduction of e173423c180dc1e4e6ee8938d9e4376a7a8b9757 subclassing EventTarget would be even harder. Also the pattern used in the issue description would not work even if WebSocket was a subclass of EventTarget.

const event = new Event('foo');

const target1 = new EventTarget();
const target2 = new EventTarget();

target1.addEventListener('foo', target2.dispatchEvent.bind(target2));
target2.addEventListener('foo', function (event) {
  console.log(event);
});

target1.dispatchEvent(event);
node:internal/event_target:635
  process.nextTick(() => { throw err; });
                           ^

Error [ERR_EVENT_RECURSION]: The event "foo" is already being dispatched
    at new NodeError (node:internal/errors:370:5)
    at EventTarget.dispatchEvent (node:internal/event_target:401:13)
    at EventTarget.[nodejs.internal.kHybridDispatch] (node:internal/event_target:455:20)
    at EventTarget.dispatchEvent (node:internal/event_target:403:26)
    at Object.<anonymous> (/home/luigi/event-target.js:11:9)
    at Module._compile (node:internal/modules/cjs/loader:1095:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1124:10)
    at Module.load (node:internal/modules/cjs/loader:975:32)
    at Function.Module._load (node:internal/modules/cjs/loader:816:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:79:12) {
  code: 'ERR_EVENT_RECURSION'
}
piranna commented 3 years ago

The event "foo" is already being dispatched

Does this means, once an event is dispatched, it can't be dispatched again in other object, and a new one needs to be created? It sorts of make sense, since target should be different...

lpinca commented 3 years ago

It can but you need to wait for the previous dispatch to complete.

lpinca commented 3 years ago

We could do something like this

diff ```diff diff --git a/lib/event-target.js b/lib/event-target.js index cc4f3ba..1da1f1a 100644 --- a/lib/event-target.js +++ b/lib/event-target.js @@ -1,21 +1,32 @@ 'use strict'; +const event = new Event('foo'); +let kTarget; + +for (const symbol of Object.getOwnPropertySymbols(event)) { + if (String(symbol) === 'Symbol(kTarget)') { + kTarget = symbol; + break; + } +} + /** * Class representing an event. * * @private */ -class Event { +class WsEvent extends Event { /** - * Create a new `Event`. + * Create a new `WsEvent`. * * @param {String} type The name of the event * @param {Object} target A reference to the target to which the event was * dispatched */ constructor(type, target) { - this.target = target; - this.type = type; + super(type); + + this[kTarget] = target; } } @@ -25,7 +36,7 @@ class Event { * @extends Event * @private */ -class MessageEvent extends Event { +class MessageEvent extends WsEvent { /** * Create a new `MessageEvent`. * @@ -46,7 +57,7 @@ class MessageEvent extends Event { * @extends Event * @private */ -class CloseEvent extends Event { +class CloseEvent extends WsEvent { /** * Create a new `CloseEvent`. * @@ -72,7 +83,7 @@ class CloseEvent extends Event { * @extends Event * @private */ -class OpenEvent extends Event { +class OpenEvent extends WsEvent { /** * Create a new `OpenEvent`. * @@ -90,7 +101,7 @@ class OpenEvent extends Event { * @extends Event * @private */ -class ErrorEvent extends Event { +class ErrorEvent extends WsEvent { /** * Create a new `ErrorEvent`. * @@ -129,22 +140,27 @@ const EventTarget = { if (typeof listener !== 'function') return; function onMessage(data, isBinary) { - listener.call( - this, - new MessageEvent(isBinary ? data : data.toString(), this) - ); + const event = new MessageEvent(isBinary ? data : data.toString(), this); + listener.call(this, event); + event[kTarget] = null; } function onClose(code, message) { - listener.call(this, new CloseEvent(code, message.toString(), this)); + const event = new CloseEvent(code, message.toString(), this); + listener.call(this, event); + event[kTarget] = null; } function onError(error) { - listener.call(this, new ErrorEvent(error, this)); + const event = new ErrorEvent(error, this); + listener.call(this, event); + event[kTarget] = null; } function onOpen() { - listener.call(this, new OpenEvent(this)); + const event = new OpenEvent(this); + listener.call(this, event); + event[kTarget] = null; } const method = options && options.once ? 'once' : 'on'; ```

but it is very fragile and event.target would not be a real EventTarget.

danieltroger commented 3 years ago

Yeah I actually figured out that I can't redispatch events anyways, not even in the browser. So nvm for my part. But it would still be better if you used something that's instanceof EventTarget and then dispatched things that are instances of native Event because then .addEventListener would be native and have options like once.

lpinca commented 3 years ago

The once option is already supported. See https://github.com/websockets/ws/commit/2e5c01f5b550ae4171d127b0b707ebcec5925cc3.

piranna commented 3 years ago
-class Event {
+class WsEvent extends Event {

👍🏻 to this :-)

lpinca commented 3 years ago

I would be open to that if Symbol('kTarget') was exposed.

danieltroger commented 3 years ago

The once option is already supported. See 2e5c01f.

Ah ok, but if I use .onmessage at the same time it doesn't seem to work IIRC

lpinca commented 3 years ago

Ah ok, but if I use .onmessage at the same time it doesn't seem to work IIRC

It works if websocket.addEventListener() is used after websocket.onmessage. It doesn't if the order is reversed but

  1. It is fixable.
  2. It is an edge case. Why using both?
  3. AFAIK it is not possible to create on<eventName> attributes with the Node.js EventTarget implementation. There is an helper function to do that but it is not public.
danieltroger commented 3 years ago

Ah ok, but if I use .onmessage at the same time it doesn't seem to work IIRC

It works if websocket.addEventListener() is used after websocket.onmessage. It doesn't if the order is reversed but

1. It is fixable.

2. It is an edge case. Why using both?

3. AFAIK it is not possible to create `on<eventName>` attributes with the Node.js `EventTarget` implementation. There is an [helper function](https://github.com/nodejs/node/blob/e2a6399be742f53103474d9fc1e56fadf7f90ccc/lib/internal/event_target.js#L651-L683) to do that but it is not public.

That's exactly my point. It's a big strange buggy mess because you tried making your own events. By using the browser/node provided events everything is compatible and you don't get weird issues. There is a, maybe limited, spec, but it's followed well in all aspects.

2. It is an edge case. Why using both?

Made a "mothersocket" that reconnects if anything goes wrong, "queues" messages and sends them so often until the server acknowledges them - and that still can be used like it's "one websocket". It runs both on browser and node and because the browser has .onmessage, etc I had to implement on-something support. I tried to just put it onto the websocket and debugged the hell out of it because I ran into the weird something added first other ones don't fire bug. But I ended up just adding one event listener and dispatching to the class itself and calling the .on functions from there.

piranna commented 3 years ago

It works if websocket.addEventListener() is used after websocket.onmessage. It doesn't if the order is reversed but

If I understood correctly the spec, using on... remove all the previous handlers, both on... and addEventListener ones. That's why on... ones are totally discouraged and unrecommended in W3C specs.

lpinca commented 3 years ago

It's a big strange buggy mess because you tried making your own events.

I respect your opinion, but I didn't do it. I've just tried to improve it. From https://github.com/websockets/ws/issues/1818#issuecomment-731575731

If I could go back in time I would have never made the WebSocket an EventTarget. Anyway the feature was added before I started contributing to ws and I think the reason was to have a browser compatible interface.

The primary, EventEmitter based interface is way better for a server-side library in my opinion. I don't think it is possible to inherit from both EventEmitter and EventTarget as that means emitting the events twice as written above. So what is in place is a "best-effort" approach to have a browser compatible API. I wasn't there but I'm pretty sure that making it spec-compliant was not even considered. If you have better ideas please share them, I'm all ears.

Also, again in my opinion, the fact that there is no way to create on<eventName> attributes when using the Node.js EventTarget implementation proves that it is not quite there yet. We would need to use a copy of defineEventHandler() or something like that.

lpinca commented 3 years ago

If I understood correctly the spec, using on... remove all the previous handlers, both on... and addEventListener ones. That's why on... ones are totally discouraged and unrecommended in W3C specs.

If that is the case, then scratch 1. It is fixable and ~~2. It is an edge case. Why using both? ~~, as it works as intended.

danieltroger commented 3 years ago

If I understood correctly the spec, using on... remove all the previous handlers, both on... and addEventListener ones. That's why on... ones are totally discouraged and unrecommended in W3C specs.

What spec? :eyes:

This is how it works in the browser and how I'm used to it:

Screenshot 2021-07-15 at 21 54 06
danieltroger commented 3 years ago

I respect your opinion, but I didn't do it. I've just tried to improve it. From #1818 (comment)

Ah shoot, np. Thanks.

Then node is the weird thing and you're just an unfortunate target of my frustration as I'm coming from the frontend.

Why the fuck do they have an EventEmitter when there's also EventTarget? Also they don't even have CustomEvent.

Also, again in my opinion, the fact that there is no way to create on attributes when using the Node.js EventTarget implementation proves that it is not quite there yet. We would need to use a copy of defineEventHandler() or something like that.

I'd just not support it. But if you want, why not just make it a normal property and every time you dispatch an event you check if the property is a function and then call it? It's like 4 lines and as far as I understand it would cover the functionality

lpinca commented 3 years ago

Why the fuck do they have an EventEmitter when there's also EventTarget?

The EventEmitter came first and is more efficient in terms of both performance and memory usage.

I'd just not support it. But if you want, why not just make it a normal property and every time you dispatch an event you check if the property is a function and then call it? It's like 4 lines and as far as I understand it would cover the functionality.

Yes, I guess that is a not spec-compliant way of doing it :)

danieltroger commented 3 years ago

The EventEmitter came first and is more efficient in terms of both performance and memory usage.

Weird. Why would they do that?

Yes, I guess that is a not spec-compliant way of doing it :)

Just using it as a property? Why is that not spec compliant? But yeah, coupled with

on... ones are totally discouraged and unrecommended in W3C specs.

I'd call for not supporting it.

lpinca commented 3 years ago

FWIW https://github.com/websockets/ws/commit/0b21c03a6e69f8e37b2dfe55c4e753575fc09ac7 fixes the ws.on<eventName> after websocket.addEventListener() issue discussed above.

danieltroger commented 3 years ago

Damn, that's awesome. Props to you! One frustration less for users.

dargmuesli commented 2 years ago

I just had a weird bug migrating to Nuxt's v3-bridge: Class extends value [object Module] is not a constructor or null coming from class WebSocket extends EventEmitter while on Node 16. My current fix is to specify the following in nuxt.config.ts:

alias: {
  ws: 'ws/browser.js',
}
lpinca commented 2 years ago

@stackdev37 it is already like that.

function foo() {}
function bar() {}

websocket.onmessage = foo;
websocket.onmessage = bar;

In the above example, the foo handler is removed when the bar handler is set.

kettanaito commented 9 months ago

This is still an issue. In fact, ws should drop event-target.js entirely. Both EventTarget and Event are provided by Node.js. Polyfilling them leads to all sort of unpredictable behaviors (see https://github.com/nodejs/undici/issues/2663) and fails Node's internal instanceof check on dispatched events, causing errors where none should be.

MessageEvent, as well as other events, must be extending the global Event class. Right now, they extend the internal custom Error class.

Can someone please share the status of this issue? If I find time to open a pull request, will somebody support me with the review and see this change merged?

piranna commented 9 months ago

I think it's time to reconsider this, all Node.js versions that don't support EventTarget are largely unmaintained...

kettanaito commented 9 months ago

@piranna, replacing event-target.js with Node.js globals should be rather straightforward, given ws didn't implement any custom functionality. I'd pretty much like to see this change released to have good examples for WebSocket API interception in MSW. I think ws is great!

lpinca commented 9 months ago

Removing the EventEmitter interface is not an option. Having both the EventEmitter interface and the native EventTarget interface is not easy. I would rather remove the EventTarget interface from ws.

kettanaito commented 9 months ago

@lpinca any particular reason the WebSocket class extends EventEmitter and not EventTarget (apart from historical reasons, of course)?

It can benefit greatly from extending EventTarget instead. In fact, the source code goes at length to adhere to EventTarget (since that's the interface a WebSocket instance expects) due to extending EventEmitter:

There are likely more, I haven't looked much. This is a fundamental issue since WebSocket extends EventTarget by design, so it will always be easier to implement such a class extending EventTarget.

I'd even argue the convenience of the .on()/.off() methods doesn't justify the cost but if one wishes to keep that public API, it's more straightforward to implement those custom methods through addEventListener()/removeEventListener() of the event target.

kettanaito commented 9 months ago

I propose the following:

  1. Remove event-target.js module entirely. Everything it implements is available natively in Node.js now.
  2. Keep WebSocket extends EventEmitter to keep the change area to a minimum.
  3. Move the addEventListener and removeEventListener custom mapping from event-target.js to websocket module directly. This is the only place it's being used (to map .emit() calls to proper corresponding dispatchEvent() calls).

Release this. Then, discuss anything else.

@lpinca, what do you think about this proposal?

lpinca commented 9 months ago

any particular reason the WebSocket class extends EventEmitter and not EventTarget (apart from historical reasons, of course)?

Performance, and the ability to pass multiple arguments to the listeners. This allows for example to do

websocket.on('message', function (buffer, isBinary) {
  // Let the user decide what to do here.
});

without calling buffer.toString() every time for text messages. This is useful for streaming and for cases like this

websocket.on('message', function (buffer, isBinary) {
  websocket.send(buffer, { binary: isBinary });
});
  1. Move the addEventListener and removeEventListener custom mapping from event-target.js to websocket module directly. This is the only place it's being used (to map .emit() calls to proper corresponding dispatchEvent() calls).

What is the difference? We can't extend the native Event class to create the MessageEvent, OpenEvent, and CloseEvent, and even if we could, there would still be incompatibilities, see https://github.com/websockets/ws/issues/1818#issuecomment-880689446.

kettanaito commented 9 months ago

without calling buffer.toString() every time for text messages

Isn't the transferred data either text or buffer-like? I know this is a naive suggestion but when does typeof data === 'string' ? data : data.toString() falls short?

We can't extend the native Event class to create the MessageEvent, OpenEvent, and CloseEvent, and even if we could, there would still be incompatibilities, see

I set the target on the WebSocket events using this bindEvent function:

export function bindEvent<E extends Event, T>(
  target: T,
  event: E
): EventWithTarget<E, T> {
  Object.defineProperty(event, 'target', {
    enumerable: true,
    writable: false,
    value: target,
  })
  return event as EventWithTarget<E, T>
}

I have the luxury of not caring about the Node internal that much but for ws you can set the necessary symbols here to represent the target. I need to look at how Node represents it and whether that's an internal symbol we have no effect on. Then it would indeed be problematic.

lpinca commented 9 months ago

I know this is a naive suggestion but when does typeof data === 'string' ? data : data.toString() falls short?

That is how it worked and was changed to not call data.toString() every time. It is unnecessary overhead sometimes. See https://github.com/websockets/ws/commit/e173423c180dc1e4e6ee8938d9e4376a7a8b9757.

I need to look at how Node represents it and whether that's an internal symbol we have no effect on. Then it would indeed be problematic.

It is indeed an internal Symbol. See https://github.com/nodejs/node/blob/f3fcad280477061ab99803875069757ae117a485/lib/internal/event_target.js#L63 and https://github.com/nodejs/node/blob/f3fcad280477061ab99803875069757ae117a485/lib/internal/event_target.js#L184-L188.

lpinca commented 9 months ago

We could override the getters in the subclass but that would prevent event.target from returning the correct target if the event is dispatched again via eventTarget.dispatchEvent(). At least an error is thrown now.

laurisvan commented 4 weeks ago

I am not 100% sure if my observation is the same, but we started using native EventEmitter.addListener(, this) in order to reduce unnecessary context creation and solve potential memory leaks (see e.g. https://jakearchibald.com/2024/garbage-collection-and-closures/ and https://webreflection.medium.com/dom-handleevent-a-cross-platform-standard-since-year-2000-5bf17287fd38). While it works great for e.g. 'message' event, we quickly noticed it won't work for 'pong'.

It appears part of the events are sent using the EventEmitter, part of them use some other implementation and require on(event, listener) type of bindings.

Would it be possible to send also pong events through EventEmitter API? I noticed 'pong' is not part of native WebSocket events (see https://developer.mozilla.org/en-US/docs/Web/API/WebSocket#events) so I could not use it as an argument there. Regardless, it would be a nice addition to us.

lpinca commented 4 weeks ago

All events are emitted via emitter.emit().

https://github.com/websockets/ws/blob/019f28ff1ffddfcdc428d1de5ecd98648057a2ab/lib/websocket.js#L1243

laurisvan commented 4 weeks ago

Sorry, then it must have be me misreading the typings. On the second look, it indeed seems that 'pong' is listed as part of addListener calls here (https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/ws/index.d.ts).

I was almost certain this would be the case, as our websocket code simply did not work with addListener('pong', ...) but I needed to use on('pong', ...). Perhaps the problem is somewhere else. I will see if I can isolate an easily reproducible sample.

Attached my SocketHandler class for the sake of completeness. It acts as glue code between graphql-ws and fastify. Never mind fastify.WebSocket typing - it is re-exported WebSocket.

class SocketHandler<E extends Record<PropertyKey, unknown> = Record<PropertyKey, never>> {
  static isProd = process.env.NODE_ENV === 'production'
  static eventNames = ['error', 'ping', 'pong', 'close', 'message'] as const

  pingInterval: NodeJS.Timeout | null
  emittedErrorHandled: boolean = false
  pongWait: ReturnType<typeof setTimeout> | null = null
  pongListener: ((...args: any) => void) | null = null

  // Runtime bound callbacks
  messageCallback: ((data: string) => Promise<void>) | null = null
  closed: ((code?: number | undefined, reason?: string | undefined) => Promise<void>) | null = null

  constructor(private readonly socket: fastifyWebsocket.WebSocket, private readonly keepAlive: number = 12_000) {
    // Bind event handlers. Never mind the type errors - socket is EventEmitter and uses the handleEvent method
    for (const event of SocketHandler.eventNames) {
      socket.addEventListener(event as any, this as any)
    }

    // For an unknown reason, we must respond pong this way. Pong simply won't get emitted via
    // SocketHandler.handleEvent
    this.pongListener = this.handlePong.bind(this)
    socket.on('pong', this.pongListener as (...args: any) => void)

    this.pingInterval =
      keepAlive > 0 && isFinite(keepAlive)
        ? setInterval(() => {
            this.handlePing()
          }, keepAlive)
        : null
  }

  async send(data): Promise<void> {
    if (this.socket.readyState !== this.socket.OPEN) {
      return
    }

    return new Promise((resolve, reject) => {
      this.socket.send(data, (err) => {
        err ? reject(err) : resolve()
      })
    })
  }

  open(server: Server<Extra & Partial<E>>, request: FastifyRequest) {
    this.closed = server.opened(
      {
        protocol: this.socket.protocol,
        send: async (data: string) => this.send(data),
        close: async (code, reason) => {
          this.close(code, reason)
        },
        onMessage: (callback) => {
          this.messageCallback = callback
        }
      },
      // eslint-disable-next-line @typescript-eslint/consistent-type-assertions
      { socket: this.socket, request } as Extra & Partial<E>
    )
  }

  close(code, reason) {
    this.socket.close(code, reason)
  }

  handleError(err: Error, checkEmitErrors = true) {
    if (checkEmitErrors && this.emittedErrorHandled) {
      return
    }
    this.emittedErrorHandled = true

    console.error('Internal error emitted on a WebSocket socket. Please check your implementation.', err)
    this.socket.close(
      CloseCode.InternalServerError,
      SocketHandler.isProd
        ? 'Internal server error'
        : SocketHandler.limitCloseReason(err instanceof Error ? err.message : String(err), 'Internal server error')
    )
  }

  async handleMessage(data: WebSocket.Data) {
    try {
      await this.messageCallback?.(String(data))
    } catch (err) {
      this.handleError(err, false)
    }
  }

  handlePing() {
    // Ping pong on open sockets only
    if (this.socket.readyState === this.socket.OPEN) {
      if (this.pongWait) {
        clearTimeout(this.pongWait)
      }

      // Terminate the connection after pong wait has passed because the client is idle
      this.pongWait = setTimeout(() => {
        this.handleTerminate()
      }, this.keepAlive)

      this.socket.ping()
    }
  }

  handlePong() {
    if (this.pongWait) {
      clearTimeout(this.pongWait)
      this.pongWait = null
    }

    // Note: WS sends pong automatically, so no need to do it manually
  }

  handleClose(code, reason) {
    if (this.pongWait) {
      clearTimeout(this.pongWait)
    }

    if (this.pingInterval) {
      clearInterval(this.pingInterval)
    }

    if (
      !SocketHandler.isProd &&
      code === CloseCode.SubprotocolNotAcceptable &&
      this.socket.protocol === DEPRECATED_GRAPHQL_WS_PROTOCOL
    ) {
      console.warn(
        `Client provided the unsupported and deprecated subprotocol "${this.socket.protocol}" used by subscriptions-transport-ws.` +
          'Please see https://www.apollographql.com/docs/apollo-server/data/subscriptions/#switching-from-subscriptions-transport-ws.'
      )
    }

    // Remove all listeners
    this.socket.off('pong', this.pongListener as (...args: any) => void)
    for (const event of SocketHandler.eventNames) {
      this.socket.removeEventListener(event as any, this as any)
    }

    // eslint-disable-next-line @typescript-eslint/no-floating-promises
    this.closed?.(code, String(reason))
  }

  handleTerminate() {
    this.socket.terminate()
  }

  handleEvent(event: Event) {
    switch (event.type) {
      case 'error': {
        this.handleError((event as ErrorEvent).error)
        return
      }
      case 'message': {
        // eslint-disable-next-line @typescript-eslint/no-floating-promises
        this.handleMessage((event as MessageEvent).data)
        return
      }
      case 'ping': {
        this.handlePing()
        return
      }
      case 'pong': {
        this.handlePong()
        return
      }
      case 'close': {
        const { code, reason } = event as CloseEvent
        this.handleClose(code, reason)
      }
    }
  }

  /**
   * Forked from: https://github.com/enisdenjo/graphql-ws/blob/c030ed1d5f7e8a552dffbfd46712caf7dfe91a54/src/utils.ts
   */
  static limitCloseReason(reason: string, whenTooLong: string) {
    return reason.length < 124 ? reason : whenTooLong
  }
}