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

Compression Support #272

Merged
merged 1 commit into from Dec 9, 2014
Merged

Compression Support #272

merged 1 commit into from Dec 9, 2014

Conversation

laktak
Copy link
Contributor

@laktak laktak commented Dec 2, 2014

I've added compression support (for zip & zlib) using https://github.com/imaya/zlib.js.

This also fixes decompression for large data (images) that jxg.js couldn't handle (and a compData vs compdata typo).

I'm not sure how you want to handle the dependency so I've included the source like you did with jxg.

@toberndo
Copy link
Member

toberndo commented Dec 3, 2014

Great. This fixes an old compatibility issue.

I'm not sure how you want to handle the dependency so I've included the source like you did with jxg.

Currently the sources of dependencies are included but I think we want to move away from that and use npm dependencies as with the new "es6-promise" module. A "grunt copy" task could then move the files to src/compression/

@tanx
Copy link
Member

tanx commented Dec 3, 2014

Currently the sources of dependencies are included but I think we want to move away from that and use npm dependencies as with the new "es6-promise" module. A "grunt copy" task could then move the files to src/compression/

We wouldn't even need to copy the files. The browserify build task looks for the common.js modules in the node_modules directory automatically to resolve dependencies. E.g. see here:

https://github.com/openpgpjs/openpgpjs/blob/master/src/openpgp.js#L46

This dependency is simply provided by the package.jsonhere:

https://github.com/openpgpjs/openpgpjs/blob/master/package.json#L49

@toberndo
Copy link
Member

toberndo commented Dec 3, 2014

We wouldn't even need to copy the files. The browserify build task looks for the common.js
modules in the node_modules directory automatically to resolve dependencies.

Not sure this will work, becaues in the zlib.js package only "main": "./bin/node-zlib.js", is defined, but we require:

Zlib = require('../compression/zlib.min.js'),
RawInflate = require('../compression/rawinflate.min.js'),
RawDeflate = require('../compression/rawdeflate.min.js');

@laktak
Copy link
Contributor Author

laktak commented Dec 4, 2014

@tanx , @toberndo so should I switch zlib to npm + grunt?

@tanx
Copy link
Member

tanx commented Dec 4, 2014

@toberndo I see. Well yes then I guess a grunt copy step is necessary.

@Iaktak that would be good. Then we wouldn't have third party builds in the openpgpjs src.

Thanks for the PR by the way. I'm interested to see how this will effect performance.

@laktak
Copy link
Contributor Author

laktak commented Dec 4, 2014

I've updated the PR. Please take a look a the grunt task though, I usually do gulp ;)

@tanx
Copy link
Member

tanx commented Dec 4, 2014

Could you squash (git rebase -i) the commits so that the libs are not in the git history? Thx

…ya/zlib.js

add zlibjs dependency, grunt task
fix compData vs compdata bug
@laktak
Copy link
Contributor Author

laktak commented Dec 4, 2014

Should be fixed.

@tanx
Copy link
Member

tanx commented Dec 5, 2014

Thx!

@toberndo you have more insight into the compression code than me. Could you review the code? I'll be happy to create a new release after that.

@toberndo
Copy link
Member

toberndo commented Dec 8, 2014

Did some performance testing and the numbers we get are really nice.

Decompression, 5 MB file:

v0.8.2

zip    2391 ms
zlib   3320 ms

with zlib.js

zip    497 ms
zlib   391 ms

I also verified that #55 is fixed with this PR.

Ready to merge.

@tanx
Copy link
Member

tanx commented Dec 9, 2014

@toberndo sound great! Thanks for looking at this

I'll merge and release as 0.9

tanx pushed a commit that referenced this pull request Dec 9, 2014
@tanx tanx merged commit bed3930 into openpgpjs:master Dec 9, 2014
@tanx
Copy link
Member

tanx commented Dec 9, 2014

Released v0.9.0 on npm and minified build here: https://github.com/openpgpjs/openpgpjs/releases/tag/v0.9.0

Great job guys!

@planeguy
Copy link

Noob question: How do you use this compression for outgoing encrypted messages? openpgp.config.compresson?

@PawelGorny
Copy link
Contributor

Do you have any junit test to compare the performance?

@toberndo
Copy link
Member

@planeguy Compression for outgoing encrypted messages is not yet supported in the high level API.

@PawelGorny I pushed my unit test here: mailvelope@6be3e5f

@laktak laktak deleted the compress branch December 16, 2014 23:19
@barrydegraaff
Copy link

This sounds good, would it be possible to include a (single file) build of zlib with the release of openpgpjs, so we don't all need to build that ourselves?

The same as been done for the openpgp.worker.js?

@toberndo
Copy link
Member

@barrydegraaff zlib is already part of dist/openpgp.js and dist/openpgp.min.js. You just need to fetch it with npm update before doing the build with grunt.

@alexluft
Copy link

Absolutely awesome! Is someone working on adding compression support for outgoing messages to high level API? Some kind of config switch?

@puzrin
Copy link

puzrin commented Apr 13, 2015

I'd recomment to take a look at https://github.com/nodeca/pako for inflate/deflate

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

9 participants