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

compress .gif file error #89

Closed
qlqllu opened this issue Feb 10, 2014 · 18 comments
Closed

compress .gif file error #89

qlqllu opened this issue Feb 10, 2014 · 18 comments

Comments

@qlqllu
Copy link

qlqllu commented Feb 10, 2014

I want to compress a .gif file by using jszip in node js, the code is like this:

var fileContent = fs.readFileSync("loading.gif", 'base64');
zip.file('loading.gif', fileContent, {base64: true});
var data = zip.generate({type: 'nodebuffer', compression: 'DEFLATE'});
fs.writeFileSync( '/loading.zip', data, 'binary');

The code can run and the zip file can be generated. However, the .gif file can't be decompressed correctly.
loading

@Mithgol
Copy link
Contributor

Mithgol commented Feb 10, 2014

What would happen if you make the following replacements:

  • fs.readFileSync("loading.gif", 'base64')fs.readFileSync('loading.gif', {encoding: 'base64'})
  • fs.writeFileSync( '/loading.zip', data, 'binary')fs.writeFileSync( '/loading.zip', data, {encoding: 'binary'})

The idea is to pass options as an object, like Node.js API docs currently say.

@Mithgol
Copy link
Contributor

Mithgol commented Feb 10, 2014

On the second thought, just fs.writeFileSync( '/loading.zip', data) is enough: .writeFileSync ignores encoding when data is a Node.js Buffer object.

@qlqllu
Copy link
Author

qlqllu commented Feb 10, 2014

@Mithgol
I have tried as what you suggest, but the same error.
Could you have a test and check the reason?

I think the reason is "DEFLATE" compression error. because if I don't use compression: 'DEFLATE' option , i.e.var data = zip.generate({type: 'nodebuffer'});, the zip format is correct.

Thanks!

@Mithgol
Copy link
Contributor

Mithgol commented Feb 10, 2014

Okay, wait a bit.

@Mithgol
Copy link
Contributor

Mithgol commented Feb 10, 2014

I've tested with the following code:

var fs = require('fs');
var jsZIP = require('../../');
var zip = new jsZIP();
var fileContent = fs.readFileSync('animated.gif', {encoding: 'base64'});
zip.file('animated.gif', fileContent, {base64: true});
var data = zip.generate({type: 'nodebuffer', compression: 'DEFLATE'});
fs.writeFileSync('animated.zip', data);

The file animated.gif in the compressed animated.zip is exactly the same as the source.

@Mithgol
Copy link
Contributor

Mithgol commented Feb 10, 2014

I also had a Travis CI test.

See the JavaScript code of the test, and animated GIF file, and ZIP archive in my repository, branch animated-gif-test.

See also the Travis CI log where the test passes (lines 898—907).

@Mithgol
Copy link
Contributor

Mithgol commented Feb 10, 2014

The test verifies that the generated ZIP file (on Travis) would be equal (if written) to the contents of the existing file animated.zip from my repository.

@qlqllu Can you checkout my branch animated-gif-test and then in its root directory (where package.json resides) run npm install --production and npm install mocha and npm run test-animated-gif and see if the test passes? If it passes, can you unpack animated.zip and confirm that the unpacked animated.gif is the same as the source animated.gif file?

@dduponchel
Copy link
Collaborator

Same as @Mithgol, I can't reproduce the issue (using the animated-gif-test branch). Maybe the upload on github changed the image ? (optimizing the image to save bandwidth ?)

@qlqllu
Copy link
Author

qlqllu commented Feb 11, 2014

@Mithgol
Thank you very much! I have passed your test, it works well.
So, I think it's my fault. In fact, I use node-zip to zip the files, so, I think the reason is that the node-zip use an old version of jszip.

However, I can't use jszip directly, because if I add jszip to my dependencies, the npm install don't work successfully, whether I use "jszip": "https://github.com/Stuk/jszip" or "jszip": "2.1.0".

This is the error message:

npm ERR! not a package C:\Users\juns6831.CHN\AppData\Local\Temp\npm-6012-w1S_QOrf\1392083447552-0.5666130546014756\tmp.tgz
npm ERR! Error: ENOENT, open 'C:\Users\juns6831.CHN\AppData\Local\Temp\npm-6012-w1S_QOrf\1392083447552-0.5666130546014756\package\package.json'

@Mithgol
Copy link
Contributor

Mithgol commented Feb 11, 2014

@qlqllu

I have an idea of a workaround.

Try adding "jszip": "https://github.com/Stuk/jszip/tarball/master" to your dependencies.

The error messages you've seen (not a package especially) are caused by npm that expects HTTPS URLs to be URLs of tarballs (not just of repositories) in package.json. See also “URLs as Dependencies” section of the corresponing npm docs.

@Mithgol
Copy link
Contributor

Mithgol commented Feb 11, 2014

On the second thought, use "jszip": "https://github.com/Stuk/jszip/tarball/6c1d9a5f0c1e4033258534172a70ff4b69016bd2" to be more exact.

@qlqllu
Copy link
Author

qlqllu commented Feb 11, 2014

Thank you for your quick response and professional knowledge! My problem is solved!
Looking forward to you push the code to npm registry.

Another small issue:
This code works:

var jsZIP = require('jszip');
var zip = new jsZIP();

This code doesn't work:

var jsZIP = new require('jszip')();

The error is:

    this.files = {};
               ^
TypeError: Cannot set property 'files' of undefined
    at JSZip (D:\WORK_ROOT\GitHub\arcgis-webapp-builder\builder\node_modules\jszip\lib\index.js:27:16)
    at Object.<anonymous> (D:\WORK_ROOT\GitHub\arcgis-webapp-builder\builder\test\test-run-jszip.js:5:31)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Function.Module.runMain (module.js:497:10)
    at startup (node.js:119:16)
    at node.js:901:3

@Mithgol
Copy link
Contributor

Mithgol commented Feb 11, 2014

That error is caused by JavaScript's operator precedence. The operator new has higher precedence than a function call.

It can be worked around by regrouping the operators:

var jsZIP = new (require('jszip'))()

A more elegant way would be eliminating the need for the new operator; we could use John Resig's self-calling constructor as a pattern for that. (After all, nobody calls new $ with John Resig's jQuery.)

Let me see if I can make a pull request for that.

@Mithgol
Copy link
Contributor

Mithgol commented Feb 11, 2014

I've opened the corresponding pull request: #93.

You should be able to use require('jszip')() instead of new (require('jszip'))() after and if my patch lands.

@Stuk
Copy link
Owner

Stuk commented Feb 11, 2014

Looks like this issue is solved. Thanks everyone :)

@Stuk Stuk closed this as completed Feb 11, 2014
@dduponchel
Copy link
Collaborator

Nice to see that your issue has been resolved @qlqllu !
On an unrelated note, you can read nodejs Buffer directly with JSZip (since v2.0.0), so :

var fileContent = fs.readFileSync("loading.gif");
zip.file('loading.gif', fileContent);
var data = zip.generate({type: 'nodebuffer', compression: 'DEFLATE'});
fs.writeFileSync( 'loading.zip', data);

will work and should be faster :)

@qlqllu
Copy link
Author

qlqllu commented Feb 13, 2014

@dduponchel
Thanks! This is works well for me.

@Mithgol
Copy link
Contributor

Mithgol commented Feb 13, 2014

I've deleted my above mentioned branch: it's no longer necessary after the issue is closed.

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

No branches or pull requests

4 participants