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

Use yazl instead of JSZip #42

Merged
merged 1 commit into from
Mar 27, 2015
Merged

Use yazl instead of JSZip #42

merged 1 commit into from
Mar 27, 2015

Conversation

kevva
Copy link
Contributor

@kevva kevva commented Feb 15, 2015

Also adds a test to check whether the zip has been created successfully or not by checking the contents.

@kevva
Copy link
Contributor Author

kevva commented Feb 15, 2015

Here are some benchmarks using this file https://gist.github.com/kevva/54d8c3d62a2bba256df8:

                      Create ZIP archive
             192 op/s » jszip
             900 op/s » yazl

@sindresorhus
Copy link
Owner

@kevva You told me to wait on merging this, but I can't remember the reason?

Merge conflict also needs to be fixed.

@kevva
Copy link
Contributor Author

kevva commented Mar 12, 2015

It seems to work fine by looking at the tests. I remember trying to extract the file using the archive manager on Ubuntu and it failed for some reason though. Using unzip worked though.

@sindresorhus
Copy link
Owner

@kevva Any open issue on yazl?

I don't want to use it if it has bugs as I will have to deal with them and JSZip seems to be working ok.

@kevva
Copy link
Contributor Author

kevva commented Mar 13, 2015

Nope, but I keep getting random write after end when testing now so I'll have to look into this further.

@rickychien
Copy link

Vote for yazl!

@kevva I ran into the same issue that my zip cannot be unzip properly Error 1 - operation not permittedby Archive Utility (default on Mac) but worked with The unarchiver or unzip.

Also adds a test to check whether the zip has been created successfully or not.

Fixes #38.
@kevva
Copy link
Contributor Author

kevva commented Mar 26, 2015

@sindresorhus, I think this should be fine now. Tested and it seems to work with the Archive Manager (in Ubuntu) too.

@rickychien
Copy link

thejoshwolfe/yazl#14

It seems that OS X's issue has been fixed hours ago. It's time to merge this PR!

kevva added a commit that referenced this pull request Mar 27, 2015
Use `yazl` instead of `JSZip`
@kevva kevva merged commit 7258eb7 into sindresorhus:master Mar 27, 2015
@sindresorhus sindresorhus deleted the yazl branch March 27, 2015 12:22
@kevva
Copy link
Contributor Author

kevva commented Mar 27, 2015

💃 More than 4x speed improvement!


cb();
zip.end(function () {
zip.outputStream.pipe(concatStream(function (data) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't have to wait until the end callback to pipe the output stream. in fact, you can skip the zip.end function entirely - it's just to notify you when it knows the final size of the zip file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think it's needed. I get timeout issues otherwise when running tests.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh sorry, you do need to call it. but you don't need to wait for the callback.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Data becomes available in this stream soon after calling one of addFile(), addReadStream(), or
addBuffer(). Clients can call pipe() on this stream at any time, such as immediately after getting
a new ZipFile instance, or long after calling end().

from https://github.com/thejoshwolfe/yazl#outputstream

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, makes no difference in speed or whatsoever though since we're only handling one buffer at a time.

@rickychien
Copy link

Could we update yazl to version 2.2.1 since it fixed on OS X
See also thejoshwolfe/yazl#14

@kevva
Copy link
Contributor Author

kevva commented Mar 28, 2015

@rickychien, ^2.1.0 will install 2.2.1.

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.

4 participants