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

`D` attribute missing from folder when a file in a subfolder is added #154

Closed
isochronous opened this Issue Jul 17, 2014 · 14 comments

Comments

Projects
None yet
3 participants
@isochronous
Contributor

isochronous commented Jul 17, 2014

In a case like this:

var zip = new JSZip();
zip.file('subdir/test.txt', 'a test file');
/// ... etc

When the created zip is opened in 7zip or another zip program that shows the attributes of each entry in the zip file, the subdir folder exists, but the D (directory) attribute is not present. This causes problems for decompression libraries that use that attribute to check for the presence of directories within the zip. The .folder method seems to create the attribute, however.

In addition, the Method listed for folders should generally be store (vs deflate for files), but is empty when executing the above example.

@dduponchel

This comment has been minimized.

Show comment
Hide comment
@dduponchel

dduponchel Jul 17, 2014

Collaborator

zip.file('subdir/test.txt', 'a test file'); creates only one entry in the zip file : subdir/test.txt. 7zip doesn't show the attributes / compression for subdir because it doesn't exist.
zip.folder('subdir').file('test.txt', 'a test file'); will create 2 entries (the file and the folder) and 7zip will show the D attribute.
JSZip used to create entries for subfolders automatically but changed with #130. It seems my conclusion was wrong.
I haven't played with gulp-zip but with sindresorhus/gulp-zip#29, do you get folders as well ? If not, we can add an option to get the old behavior back.

Collaborator

dduponchel commented Jul 17, 2014

zip.file('subdir/test.txt', 'a test file'); creates only one entry in the zip file : subdir/test.txt. 7zip doesn't show the attributes / compression for subdir because it doesn't exist.
zip.folder('subdir').file('test.txt', 'a test file'); will create 2 entries (the file and the folder) and 7zip will show the D attribute.
JSZip used to create entries for subfolders automatically but changed with #130. It seems my conclusion was wrong.
I haven't played with gulp-zip but with sindresorhus/gulp-zip#29, do you get folders as well ? If not, we can add an option to get the old behavior back.

@isochronous

This comment has been minimized.

Show comment
Hide comment
@isochronous

isochronous Jul 17, 2014

Contributor

Maybe I used bad example. We're zipping up all of the folders in a directory and the files they contain using gulp-zip. The only method it uses right now is .file. When we open the zips using 7zip, we see the folders, only without the attribute. We can then navigate into those folders and see the files (and additional subfolders) and again, those subfolders are navigable yet still without the D attribute. Unfortunately, when the application we're deploying the zips to (which is a java-based android app) reads the zips, it apparently looks for anything with a D attribute as a starting place from which to navigate (or at least, that's our best guess at the moment, since the only difference between a working and non-working file is the presence or lack of that attribute, and the "method" property being set to "store" in the working one). Since it doesn't find any, it just returns immediately. We can unzip the files created with gulp-zip and immediately re-zip them with 7zip, winrar, etc, re-deploy, and then they work.

So, I think the answer to your question is yes, but the android app that consumes the zip doesn't really see them as folders per se.

On Jul 17, 2014, at 4:37 PM, David Duponchel notifications@github.com wrote:

zip.file('subdir/test.txt', 'a test file'); creates only one entry in the zip file : subdir/test.txt. 7zip doesn't show the attributes / compression for subdir because it doesn't exist.
zip.folder('subdir').file('test.txt', 'a test file'); will create 2 entries (the file and the folder) and 7zip will show the D attribute.
JSZip used to create entries for subfolders automatically but changed with #130. It seems my conclusion was wrong.
I haven't played with gulp-zip but with sindresorhus/gulp-zip#29, do you get folders as well ? If not, we can add an option to get the old behavior back.


Reply to this email directly or view it on GitHub.

Contributor

isochronous commented Jul 17, 2014

Maybe I used bad example. We're zipping up all of the folders in a directory and the files they contain using gulp-zip. The only method it uses right now is .file. When we open the zips using 7zip, we see the folders, only without the attribute. We can then navigate into those folders and see the files (and additional subfolders) and again, those subfolders are navigable yet still without the D attribute. Unfortunately, when the application we're deploying the zips to (which is a java-based android app) reads the zips, it apparently looks for anything with a D attribute as a starting place from which to navigate (or at least, that's our best guess at the moment, since the only difference between a working and non-working file is the presence or lack of that attribute, and the "method" property being set to "store" in the working one). Since it doesn't find any, it just returns immediately. We can unzip the files created with gulp-zip and immediately re-zip them with 7zip, winrar, etc, re-deploy, and then they work.

So, I think the answer to your question is yes, but the android app that consumes the zip doesn't really see them as folders per se.

On Jul 17, 2014, at 4:37 PM, David Duponchel notifications@github.com wrote:

zip.file('subdir/test.txt', 'a test file'); creates only one entry in the zip file : subdir/test.txt. 7zip doesn't show the attributes / compression for subdir because it doesn't exist.
zip.folder('subdir').file('test.txt', 'a test file'); will create 2 entries (the file and the folder) and 7zip will show the D attribute.
JSZip used to create entries for subfolders automatically but changed with #130. It seems my conclusion was wrong.
I haven't played with gulp-zip but with sindresorhus/gulp-zip#29, do you get folders as well ? If not, we can add an option to get the old behavior back.


Reply to this email directly or view it on GitHub.

@isochronous

This comment has been minimized.

Show comment
Hide comment
@isochronous

isochronous Jul 18, 2014

Contributor

Okay, so I found 5c91113 and modified the latest version to reverse the changes made in that commit (except for the remove changes, as we never use the remove method and I was just testing) and now the attribute is there, as is the "store" method property, and everything is happy happy. So yeah, if an option could be added to enable automatic creation of subfolders, that would be REALLY helpful to me and my team. I could submit a pull request but I think I might inadvertently regress a couple of bug fixes you've made since the commit in question.

Contributor

isochronous commented Jul 18, 2014

Okay, so I found 5c91113 and modified the latest version to reverse the changes made in that commit (except for the remove changes, as we never use the remove method and I was just testing) and now the attribute is there, as is the "store" method property, and everything is happy happy. So yeah, if an option could be added to enable automatic creation of subfolders, that would be REALLY helpful to me and my team. I could submit a pull request but I think I might inadvertently regress a couple of bug fixes you've made since the commit in question.

@isochronous

This comment has been minimized.

Show comment
Hide comment
@isochronous

isochronous Jul 21, 2014

Contributor

The pull request basically just restores previous code, but with an added check for a createSubFolders option before creating the sub-folders. The default behavior remains unchanged.

Contributor

isochronous commented Jul 21, 2014

The pull request basically just restores previous code, but with an added check for a createSubFolders option before creating the sub-folders. The default behavior remains unchanged.

@isochronous

This comment has been minimized.

Show comment
Hide comment
@isochronous

isochronous Jul 21, 2014

Contributor

Note that a bunch of the dev dependencies don't install properly on my (windows) machine, so I was unable to run the grunt build task - that still needs to be done before publishing the new version to npm.

Contributor

isochronous commented Jul 21, 2014

Note that a bunch of the dev dependencies don't install properly on my (windows) machine, so I was unable to run the grunt build task - that still needs to be done before publishing the new version to npm.

@isochronous

This comment has been minimized.

Show comment
Hide comment
@isochronous

isochronous Jul 21, 2014

Contributor

Gah, there's only one test still failing, and that's the "Delete nested folders from relative path" test. This wouldn't be so bad if only I could actually install all of the stuff I need to test locally, but I can't, so I have to continue checking stuff in, waiting for your Travis instance to build and test it, and then iterate from there.

Contributor

isochronous commented Jul 21, 2014

Gah, there's only one test still failing, and that's the "Delete nested folders from relative path" test. This wouldn't be so bad if only I could actually install all of the stuff I need to test locally, but I can't, so I have to continue checking stuff in, waiting for your Travis instance to build and test it, and then iterate from there.

@dduponchel

This comment has been minimized.

Show comment
Hide comment
@dduponchel

dduponchel Jul 21, 2014

Collaborator

I'll take a look (and give you some feedback), that should be less painful !

Collaborator

dduponchel commented Jul 21, 2014

I'll take a look (and give you some feedback), that should be less painful !

@isochronous

This comment has been minimized.

Show comment
Hide comment
@isochronous

isochronous Jul 21, 2014

Contributor

Thanks, that's greatly appreciated.

Contributor

isochronous commented Jul 21, 2014

Thanks, that's greatly appreciated.

@Stuk Stuk added the bug label Jul 22, 2014

@isochronous

This comment has been minimized.

Show comment
Hide comment
@isochronous

isochronous Jul 23, 2014

Contributor

Just wondering, but do you have any estimate on when an updated npm package might get published?

Contributor

isochronous commented Jul 23, 2014

Just wondering, but do you have any estimate on when an updated npm package might get published?

@Stuk

This comment has been minimized.

Show comment
Hide comment
@Stuk

Stuk Jul 23, 2014

Owner

We could do a release today. What do you think about this comment? https://github.com/Stuk/jszip/pull/157/files#r15308300 We could change the API name now, but not after we've had a release.

Owner

Stuk commented Jul 23, 2014

We could do a release today. What do you think about this comment? https://github.com/Stuk/jszip/pull/157/files#r15308300 We could change the API name now, but not after we've had a release.

@isochronous

This comment has been minimized.

Show comment
Hide comment
@isochronous

isochronous Jul 23, 2014

Contributor

I think that makes sense. Do you want to handle it, or should I?

Contributor

isochronous commented Jul 23, 2014

I think that makes sense. Do you want to handle it, or should I?

@Stuk

This comment has been minimized.

Show comment
Hide comment
@Stuk

Stuk Jul 23, 2014

Owner

Would you mind updating it and doing a PR?

Owner

Stuk commented Jul 23, 2014

Would you mind updating it and doing a PR?

@isochronous

This comment has been minimized.

Show comment
Hide comment
@isochronous

isochronous Jul 23, 2014

Contributor

Sure thing, doing it now

Contributor

isochronous commented Jul 23, 2014

Sure thing, doing it now

@isochronous

This comment has been minimized.

Show comment
Hide comment
@isochronous

isochronous Jul 23, 2014

Contributor

Done, see #160

Contributor

isochronous commented Jul 23, 2014

Done, see #160

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment