Skip to content

Conversation

@vansergen
Copy link
Contributor

@vansergen vansergen commented Jan 31, 2022

@lpinca What do you think about adding this feature?

@lpinca
Copy link
Member

lpinca commented Feb 1, 2022

I'm not very happy with it. Are there big advantages over monkey patching the WebSocket class directly?

const WebSocket = require('ws');

Object.defineProperty(WebSocket.prototype, 'custom_field', {
  get() {
    return '123';
  }
});

@vansergen
Copy link
Contributor Author

vansergen commented Feb 1, 2022

Are there big advantages over monkey patching the WebSocket class directly?

I do believe so. It would give more flexibility in terms of accessing the
original methods while overwriting them

class CustomWebSocket extends WebSocket {
  #sent;
  #created_at;
  constructor(...params) {
    super(...params);
    this.#sent = 0;
    this.#created_at = new Date();
  }
  get sent() {
    return this.#sent;
  }
  get created_at() {
    return new Date(this.#created_at);
  }
  send(...params) {
    this.#sent++;
    super.send(...params);
  }
}

in the same manner as the node:http module does

import { IncomingMessage, ServerResponse, createServer } from 'node:http';
class CustomIncomingMessage extends IncomingMessage {}
class CustomServerResponse extends ServerResponse {}
createServer({
  IncomingMessage: CustomIncomingMessage,
  ServerResponse: CustomServerResponse
}).on('request', (request, response) => {
  console.log(request instanceof CustomIncomingMessage);
  console.log(response instanceof CustomServerResponse);
});

In this way one can modify WebSocket classes per server (without modifying the original class)

import { createServer } from 'http';
import { parse } from 'url';
import { WebSocketServer, WebSocket } from 'ws';

class WebSocket1 extends WebSocket {}
class WebSocket2 extends WebSocket {}

const server = createServer();
const wss1 = new WebSocketServer({ WebSocket: WebSocket1, noServer: true });
const wss2 = new WebSocketServer({ WebSocket: WebSocket2, noServer: true });
const wss3 = new WebSocketServer({ WebSocket, noServer: true });

wss1.on('connection', function connection(ws) {
  // console.log(ws instanceof WebSocket1)
});
wss2.on('connection', function connection(ws) {
  // console.log(ws instanceof WebSocket2)
});
wss3.on('connection', function connection(ws) {
  // console.log(ws instanceof WebSocket)
});

server.on('upgrade', function upgrade(request, socket, head) {
  const { pathname } = parse(request.url);
  if (pathname === '/foo') {
    wss1.handleUpgrade(request, socket, head, function done(ws) {
      wss1.emit('connection', ws, request);
    });
  } else if (pathname === '/bar') {
    wss2.handleUpgrade(request, socket, head, function done(ws) {
      wss2.emit('connection', ws, request);
    });
  } else {
    wss3.handleUpgrade(request, socket, head, function done(ws) {
      wss3.emit('connection', ws, request);
    });
  }
});
server.listen(8080);

@lpinca
Copy link
Member

lpinca commented Feb 1, 2022

Hmm, ok. It might break if the class does not extend WebSocket and I do not like to keep adding options but it is reasonable.

vansergen and others added 3 commits February 1, 2022 14:13
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
vansergen and others added 2 commits February 1, 2022 14:38
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
@lpinca lpinca merged commit e1ddacc into websockets:master Feb 1, 2022
@lpinca
Copy link
Member

lpinca commented Feb 1, 2022

Thank you.

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

Successfully merging this pull request may close these issues.

2 participants