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

Exposed createEncodeStream and createDecodeStream #45

Closed
wants to merge 2 commits into from

Conversation

mpcsh
Copy link

@mpcsh mpcsh commented Jul 29, 2016

Hey kawanet!

Here at keybase, we've been using msgpack-lite for a few weeks in one of our projects. Long story short, we need streaming msgpack encoding and decoding per our encryption spec, and your library has been a breeze to use. Recently, we finished up testing on server side within node and are working on the browser side, where we quickly encountered a problem: the msgpack.min.js file doesn't expose the createEncodeStream and createDecodeStream functions. Here I've done nothing more than expose them in browser.js, which fixes our issues.

We can maintain our own fork of msgpack-lite, but I figured it would be easier to have it upstream for such a small change, and that way we don't have to worry about versioning in the future. I appreciate that my change inflates the size of msgpack.min.js to 89K, so if you feel that that's an issue, I can definitely separate this out into two .min.js files, the original and this new one.

Thanks for your time!

Mark and the Keybase team

@kawanet
Copy link
Owner

kawanet commented Aug 18, 2016

I appreciate that my change inflates the size of msgpack.min.js to 89K

Right. That is the reason for browser.js not to have create**codeStream methods.

However, on another project, I was faced with the same problem of lack of Stream support.

I'm working on another side to decrease .min.js file size by removing Buffer shim which is enough fat, by the way.

On some use cases, the raw Uint8Array is enough. But Buffer shim and Stream support is not needed.
I'll add lighter weight version which does not have both Buffer and Stream for browsers.
msgpack-without-buffer.min.js? msgpack-liter.min.js? msgpack-min.min.js?

On another use cases, Buffer shim and Stream support is needed.
I'd love to merge your patch to support Stream on msgpack.min.js in contrast, then.

@mpcsh
Copy link
Author

mpcsh commented Aug 22, 2016

Thanks for the response 👍 - let me know if I need to clean anything more up (not sure what the Travis failures are but they look unrelated). I'd also be willing to contribute some kind of msgpack-raw.min.js file, if you'd like. Let me know - otherwise, merge whenever is convenient. Thanks! (cc @oconnor663)

@mpcsh mpcsh closed this Aug 7, 2017
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.

None yet

2 participants