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

feat: drop readable-stream and buffer in favour of node_stream/web and UInt8Array #151

Open
robertsLando opened this issue Jan 31, 2025 · 12 comments

Comments

@robertsLando
Copy link
Member

robertsLando commented Jan 31, 2025

This would be the first part of the work needed to make MQTTjs full browser compatible without polifilly and also improve compatibility of libraries with bun/deno. I would like to know your opinion about this @mcollina

Useful resources:

@mcollina
Copy link
Member

It’s not a good idea to remove node stream support. For Node.js client and server side usage, we need them to have decent performance.

Moreover we need the bl module to avoid massive reallocations when adding new things to the packet. The current algorithm does not allocate the full packet size, but keep the individual parts separate and then write them directly to the stream.

In other words, to have decent performance on Node.js, we have to keep those.

I have no problems in adding a more whatwg friendly implementation here so that browser MQTT.js can use the proper standards.

@robertsLando
Copy link
Member Author

I have no problems in adding a more whatwg friendly implementation here so that browser MQTT.js can use the proper standards.

Could you explain me more in details what you mean with this? Does using WebStreams instead of readable-stream in browsers have any benefits?

@mcollina
Copy link
Member

Does using WebStreams instead of readable-stream in browsers have any benefits?

Bundle size would be significantly lower.

@robertsLando
Copy link
Member Author

Bundle size would be significantly lower.

Ok so why not doing this on readable-stream package instead?

@mcollina
Copy link
Member

mcollina commented Feb 3, 2025

Ok so why not doing this on readable-stream package instead?

I don't understand the question. WebStreams and Node.js streams have incompatible APIs and semantics.

@robertsLando
Copy link
Member Author

WebStreams and Node.js streams have incompatible APIs and semantics

My idea was to create a browser export of readable-stream that relies on WebStreams, I never tried WebStreams so I dunno how compatible they are with Node.js streams, sorry for the noob question.

That said, I think that adding a more whatwg friendly implementation here could be a good starting point, thanks :)

About the switch from Buffer to UInt8Array are you +1 or not? Ref: https://sindresorhus.com/blog/goodbye-nodejs-buffer

@mcollina
Copy link
Member

mcollina commented Feb 3, 2025

About the switch from Buffer to UInt8Array are you +1 or not

We can't, due to how bl works.

@robertsLando
Copy link
Member Author

@mcollina Yeah I know but I'm open to create a PR to bl as well if you think there could be some benefits.

@mcollina
Copy link
Member

mcollina commented Feb 3, 2025

I don't really understand what the benefit of that would be, the goal of bl is to implement the API of Buffer over a list of buffers.

@robertsLando
Copy link
Member Author

robertsLando commented Feb 3, 2025

The benefits of switching to UInt8Array are described here: https://sindresorhus.com/blog/goodbye-nodejs-buffer.

But if you think that's not worth it I understand

@mcollina
Copy link
Member

mcollina commented Feb 3, 2025

What I mean is that it's 100% out of scope for bl, because that exposes the Buffer API and aims to be compatible with Buffer. Therefore you'd need an equivalent module for UInt8Array.

There are also

const result = this._list.slice(this._pos, end)
, which will impact performance if converted to a UInt8Array.

@robertsLando
Copy link
Member Author

Ok, I understand. Sorry for the misunderstanding, I had these things in mind for long and wanted your opinion on this given your deep understanding of streams on Node.js side.

So if it's ok for you I would like more details about:

adding a more whatwg friendly implementation here so that browser MQTT.js can use the proper standards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants