fastify/fastify-websocket

Add readableObjectMode as an option for createWebSocketStream

aelnaiem opened this issue ยท 6 comments

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the feature has not already been requested

๐Ÿš€ Feature Proposal

Adding readableObjectMode as an option for fastify-websocket.

I noticed that currently in the documentation, it says that fastify-websocket supports objectMode as an option for [ws](https://github.com/websockets/ws/), but objectMode is not listed in the ws options documentation.

It seems reasonable to add readableObjectMode as an option to fastify-websocket as it is an option for createWebSocketStream and achieves a similar goal. It may also make sense to allow users to change other connection (duplex) options like readableHighWaterMark and writableHighWaterMark, but I haven't needed those options yet.

Motivation

There is currently no way to set readableObjectMode and this can lead to a TypeError for certain WebSocket applications, specifically:

TypeError [ERR_INVALID_ARG_TYPE]: The "chunk" argument must be of type string or an instance of 
Buffer or Uint8Array. Received an instance of ArrayBuffer

That being said, this issue which led to the inclusion of readableObjectMode as an option for createWebSocketStream suggests there are issues with objectMode and backpressure handling, and the option could be a footgun.

This error also didn't occur when using express-ws so I wonder if it's related to how Fastify works as the issue also highlights a bug in Node.js core. I noticed that express-ws doesn't seem to call createWebSocketStream at all and so I'm curious why there is a difference in implementation and that might reflect if this feature is needed.

Example

fastify.register(require('fastify-websocket'), {
  options: { maxPayload: 1048576 },
  connectionOptions: { readableObjectMode: true } // can include other duplex options 
})

Then these options can be passed to WebSocket.createWebSocketStream

const connection = WebSocket.createWebSocketStream(socket, opts.connectionOptions)

How can we reproduce the issue?

I'll try to create a minimal reproduction

It seems that it's enough to set the socket's binary type to "arraybuffer" on the server, and then to send data as a buffer from the client.

const WebSocket = require("ws");
const fastify = require("fastify")();

fastify.register(require("fastify-websocket"));

fastify.get("/", { websocket: true }, async (connection) => {
  connection.socket.binaryType = "arraybuffer";
});

fastify.listen(3000, (err) => {
  const ws = new WebSocket("ws://localhost:" + fastify.server.address().port);

  ws.onopen = () => {
    ws.send(Buffer.from("Hello!"));
  };
});

This is actually an interesting problem! Would you like to send a PR to add this feature?

Definitely! I'll create a PR

Fixed by #186, thanks @aelnaiem , released in v4.2.1!