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

Add readableObjectMode as an option for createWebSocketStream #185

Closed
2 tasks done
aelnaiem opened this issue Mar 16, 2022 · 6 comments
Closed
2 tasks done

Add readableObjectMode as an option for createWebSocketStream #185

aelnaiem opened this issue Mar 16, 2022 · 6 comments
Labels
bug Confirmed bug good first issue Good for newcomers

Comments

@aelnaiem
Copy link
Contributor

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)
@mcollina
Copy link
Member

How can we reproduce the issue?

@aelnaiem
Copy link
Contributor Author

I'll try to create a minimal reproduction

@aelnaiem
Copy link
Contributor Author

aelnaiem commented Mar 19, 2022

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!"));
  };
});

@mcollina
Copy link
Member

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

@mcollina mcollina added bug Confirmed bug good first issue Good for newcomers labels Mar 19, 2022
@aelnaiem
Copy link
Contributor Author

aelnaiem commented Mar 20, 2022

Definitely! I'll create a PR

@airhorns
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants