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

String/Buffer support? #11

Closed
puzrin opened this issue Mar 14, 2014 · 11 comments
Closed

String/Buffer support? #11

puzrin opened this issue Mar 14, 2014 · 11 comments
Labels

Comments

@puzrin
Copy link
Member

puzrin commented Mar 14, 2014

Not sure, if that's really needed. Could be convenient to pack/unpack json, for example. Possible cases:

  • deflate input:
    • ucs2 string (for example, to pack json) - can be converted to utf8 binary string
    • "binary" string (each char is [0...255])
  • deflate output:
    • (?) binary string
    • (?) base64 string
  • inflate input:
    • (?) binary string
  • inflate output:
    • binary string
    • usc2 string (for example, unpack json)
    • (?) base64 string

If anyone is strong in client-side programming, and can explain, why string conversions will be good in this lib - share your knowlege and examples in this ticket.

@puzrin puzrin added the feature label Mar 14, 2014
@creationix
Copy link

I would rather this not be part of pako. For conversions I use https://github.com/creationix/bodec/blob/master/bodec.js and there are many other such libraries available.

Ideally for me, pako would consume and emit Buffer instances in node.js and use Uint8Array in browsers.

@puzrin
Copy link
Member Author

puzrin commented Mar 14, 2014

I'm also not certain about strings support.

About Buffers - not sure that it's a good idea (also it's not good idea to replace in node zlib with pako). In worst case, if you really need to run pako in node, you can do new Buffer(Uint8Array) and new Uint8Array(Buffer). Buffers can not be used in pako directly, because pako needs fast Uint16Array & Int32Array.

@creationix
Copy link

That's fine. I can convert between node Buffer and Uint8Array when node's zlib is not an option. (I have some use cases where I need sync deflate and inflate, node seems to be async-only)

@puzrin
Copy link
Member Author

puzrin commented Mar 14, 2014

Node 0.11.12 finally got sync zlib interface. Hope to see 0.12 stable soon.

@puzrin puzrin changed the title Strings support? String/Buffer support? Mar 27, 2014
@creationix
Copy link

Once node 0.12 is released, there won't be too much reason to use pako in node.

I still have yet to see if pako can solve the streaming inflate problem I'm using Chris's code for. I'm pretty sure node's API isn't flexible enough. If that's the case and pako is flexible enough I'll still be using pako for streaming inflate in node, but I don't mind manually converting to Uint8Array for that special case.

@puzrin
Copy link
Member Author

puzrin commented Mar 27, 2014

Note, that we missed specific zlib methods https://github.com/nodeca/pako#notes , to save development time and lib size. And because i had no idea who really need those function. If you need - let me know. Adding that code is not difficult, especially if someone agree to test it in real projects :)

@creationix
Copy link

Sounds good. And once I find out which functions I need, I can even send a pull request to expose those in node too. This might be a case where digging into git's source to see which zlib functions it uses would actually be useful. Most the time I've avoided git's source because it's not very helpful.

@chrisdickinson, do you have any bandwidth to explore this? It would be nice to be able to use node's native zlib for all of js-git and pako for browser. It will be a bit before I have a chance to dig in and I would love to get a patch added to node before the 0.12 release if we do that.

@puzrin
Copy link
Member Author

puzrin commented Apr 9, 2014

Well, added strings support. It takes not as much space as i expected. And there are no slowdowns, after some black magick.

Haven't tested well hard surrogate characters, but everything else (BMP) works awesome

@puzrin puzrin closed this as completed Apr 9, 2014
@puzrin
Copy link
Member Author

puzrin commented Apr 9, 2014

PS. After long discussion in #18 buffers are considered as "useless" in pako core, adding nasty side effects. Anyway, i don't know universal way to fast copy memory block between node Buffer and Uint8Array.

Current conclusion is:

  1. Pako stays focused on native js types, super fast operations with those + chunking support at low level. It's not positioned as replacement of zlib in node.
  2. If one need intermediate abstraction of node-zlib in browser - use https://github.com/devongovett/browserify-zlib , created by @devongovett . It also supports sync operations, introduced on node 0.11+. Hope that browserify will accept it as default instead of existing shim.
  3. Others cases should consider create wrappers.
  4. Strings are supported, but limited to practical cases - binary & utf8. Just because that's convenient, not hard, and i can guarantee fast speed.

@devongovett
Copy link

browserify/browserify#721 was merged this morning, so as of now browserify-zlib/pako is the default zlib implementation for browserify.

@puzrin
Copy link
Member Author

puzrin commented Apr 9, 2014

Awesome. Now i can start list in readme, in style "Popular projects, that selected pako" :)

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

No branches or pull requests

3 participants