-
-
Notifications
You must be signed in to change notification settings - Fork 981
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
Thoughts on reducing browser bundle size #280
Comments
In fact, I went ahead and removed streaming functionality, converted |
Do you think it would be possible to retain the stream API by making a smaller readable-stream alternative? simple-peer is pretty entrenched in the "stream module" ecosystem. I think debug could be outright removed, seeing as it's only being used as a logger and not using any of debug's unique features. |
I'm not sure any alternative would be much smaller. If streaming API support is claimed, the whole API must be supported. One option might be to provide a shim for browser environments that will not contain actual implementation, but will have a few things required by simple-peer itself. Otherwise some kind of decoupling would be necessary in order to build browser version with different set of pluggable components. Another option might be to use minimal shim by default and accept duplex stream constructor as optional argument in simple-peer to upgrade it when needed. It wouldn't be difficult to supply it in Node.js. |
We could release two separate bundles (simple-peer and simple-peer-lite), the decoupling wouldn't be too difficult. The lite version could be built without those larger modules and use a non-streaming |
Problems with removing it:
We could make a minimal shim and include it in this repo so users could This seems like a lot of work. |
I'll experiment with shim and let you know how it goes |
If the cost isn't so high to maintaining the lite version in this repo, I'm open to it. Send a PR when you have something to look at @nazar-pc |
Could we somehow split core features from streaming and offer streaming version in |
Maybe we should wait until we abstract the DataChannel for multiplexing support? It might make it easier to replace Buffer/Stream dependencies. |
I don't see them as being related. Basically I expect that "light" version would only implement |
My thought was you could pass a lite version of the DataChannel wrapper that uses UInt8Array and does not have a streaming API instead of changing anything about the peer object. |
But streaming is a part of peer object's API. Could you elaborate on your idea? |
Sorry, yes. For multiplexing the plan is to have two objects: A Peer that handles signaling, media, etc, and a DataChannelWrapper that is a Duplex stream. The peer exposes these streams in events and through the We planned to keep the Peer as a Duplex by exposing the methods of a default DataChannelWrapper. So a lite version would just need to change the DataChannel implementation. ie)
|
This means any API the DataChannelWrapper has, the Peer also has as it proxies a default instance of it. |
Thanks, makes a lot of sense now. In this case I agree it might be a good idea to wait. |
Is there any news on this? There is also a stream API in the browsers now: |
I just opened an issue in the WebTorrent project for us to consider migrating from If we do this, we get the following benefits:
Take a look at the webtorrent issue for more info: webtorrent/webtorrent#1971 |
Hi, not directly related but starting from @nazar-pc's work I made a fork of It's on npm as well: The motivation was to be able to use I post this just in case it helps someone like myself. Obviously, in this repo you want to support the stream API and @feross knows the way to go. Thank you all for this excellent library ❤️ |
@mitschabaude This is excellent. I've just thought about the same, but you did it already :). Big up |
@mitschabaude thank you sooo much... i just wasted a whole day trying to figure out how to use simple-peer with snowpack |
@mitschabaude Thanks for trying this out in the fork. I've used it in my matchmaking library and am now able to use it in Snowpack and Rollup. The much slimmer bundle size is a nice bonus too. |
this repo is unfortunately kinda dead, I've made PRs to it to replace buffer with uint8 and readable with streamx, but they aren't approved yet for the meantime i made a package @thaunknown/simple-peer which preserves 100% of the functionality of simple peer with duplex streams etc, while dropping readable and buffer, which means the package is tiny, one major difference is that the output is uint8 not buffer so you can't run toString() on the outputs, rather you'd need some utility like arr2text from uint8-util to do it |
In my current app
simple-peer
is about 10% of an application size and about 20% of JS code size.Specifically, while
index.js
is 29.5KiB and one might expect that minified version would be even smaller,simplepeer.min.js
is whopping 86.5KiB right now.Before I create and start maintaining a fork of this project, I'd like to know if if this is something that can be fixed upstream.
2 packages that cause this huge size:
readable-stream
buffer
readable-stream
is valuable in Node.js, but not so much in browser environment. Is it possible that future versions ofsimple-peer
will stop using it entirely and replace with something lightweight instead? I imagine it wouldn't be too hard to implement a wrapper or even offer an optional one with this library.buffer
is a simpler one, oncereadable-stream
is eliminated we'll just need to switch fromBuffer
toUint8Array
, which is quite easy to do.Current bundle size doesn't seem tolerable to me given that it is just a wrapper around native features, so I hope for your understanding here. If these changes seem reasonable, I'd be happy to implement them myself, otherwise I'll be forced to create something like
simple-peer-light
and just maintain that fork forever.The text was updated successfully, but these errors were encountered: