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

[WebSocketServer] Allow to extend the original WebSocket class #2007

Merged
merged 7 commits into from
Feb 1, 2022
Merged

[WebSocketServer] Allow to extend the original WebSocket class #2007

merged 7 commits into from
Feb 1, 2022

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.

lib/websocket-server.js Outdated Show resolved Hide resolved
lib/websocket-server.js Show resolved Hide resolved
test/websocket-server.test.js Outdated Show resolved Hide resolved
doc/ws.md Outdated Show resolved Hide resolved
test/websocket-server.test.js Outdated Show resolved Hide resolved
@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