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

Buffer usage elimination #1 #312

Closed
wants to merge 2 commits into from
Closed

Buffer usage elimination #1 #312

wants to merge 2 commits into from

Conversation

nazar-pc
Copy link
Collaborator

This is the first step in reducing bundle size as discussed in #280

This PR removes explicit usage of Buffer API in simple-peer itself, but Buffer is still used by debug and readable-stream, so bundle size will not be much smaller just yet, those will be tacked in a separate PR.

One major implication of this PR is that data event will dispatch with Uint8Array instead of Buffer, which is a major breaking change.

I believe this is a reasonable trade-off since It is easy to upgrade existing code base with Buffer.from(data), while browser users that don't need Buffer API will have significantly less code to download.

@feross
Copy link
Owner

feross commented May 25, 2018

While I sympathize with the desire to make the browser bundle smaller, the entire reason that I initially created this package was to expose the WebRTC data channel as a Node.js stream.

If we remove Buffer and eventually break the Node.js Stream interface, then it becomes impossible to use simple-peer with the huge ecosystem of other Node.js Stream packages, and it becomes impossible to use it as a drop-in substitute for a net.Socket object like we do in WebTorrent, and others do in other P2P applications.

I think we may need to consider how to satisfy these two use cases in some other way. I'll think about it some more and share possible ideas in the original issue #280.

didlie
didlie approved these changes Jul 24, 2018
@mrlika
Copy link
Contributor

mrlika commented Feb 25, 2019

I also disappointed that huge Buffer is used everywhere. Bundles for the browser should be small. Changing the code to use compatible between Node.js and browsers ArrayBuffer/Uint8Array is the way to go.

@nazar-pc nazar-pc closed this Apr 4, 2021
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.

4 participants