-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
add createSubFolders option #157
Conversation
The zip files generated by gulp-zip do not actually contain "real" folders (that is, folders with the `D` attribute and `store` method). Opening a zip with a GUI decompression utility shows folders, but they're really "virtual" folders that are just defined by the path of the files in the zip. Extracting the zips creates the actual folder structure, but a decompression utility looking specifically for folders won't find anything, as the `D` attribute is not present. There's a pull request pending for JSZip to add an option to automatically create "real" sub-folders when using the `.file` method (Stuk/jszip#157). This commit is in anticipation of the pull request being accepted, so that gulp-zip can take advantage of the new functionality in JSZip. Including this option in calls to JSZip before the option exists won't cause any errors, so it's safe to preemptively add this to gulp-zip. Currently, the `createSubFolders` option defaults to `true`, but can be set to false by passing `createSubFolders: false` as an option to gulp-zip.
@@ -3,6 +3,8 @@ title: Changelog | |||
layout: default | |||
section: main | |||
--- | |||
### v2.3.1 2014-07-21 | |||
- Add `createSubFolders` option (see [#154](https://github.com/Stuk/jszip/issues/154)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't update the version / changelog, this will be done at release time.
Ah, I'll undo that then |
var dataType = utils.getTypeOf(data), | ||
parent; | ||
|
||
if (o && o.createSubFolders && (parent = parentFolder(name))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move this test just after the call to prepareFileAttrs
? That way, you won't have to check if o
is here.
Sure thing |
Okay, I moved that check and undid the version updates |
}); | ||
for (var i = 0; i < kids.length; i++) { | ||
delete this.files[kids[i].name]; | ||
if (file) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code comes from the previous version where all the folders where created. Now (even with this patch) we can get no result (file
is undefined) for a "virtual" folder but files in it (file("sub1/file.txt").remove("sub1")
for example). In that case, we have should keep the existing code and look for content to delete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was still kind of getting my head around the concepts involved when I was deciding how to re-implement that.... I'm not really sure what you're saying with that second sentence though.
The |
Will do |
Okay, the load script and documentation have been updated, but I'm still not really sure what to do about that |
The last error comes from |
Okay, done. I've got to go eat lunch but I'll respond to any other comments you make when I'm done. |
The master version will works but the patch won't (because it relies on existing sub folders) : you should revert the changes on this method. |
On my side, I'll go to bed. See you tomorrow ! |
There we go |
Finally, all tests passing |
`options.dir` | boolean | **Deprecated**, use `dir`. True if this is a directory | ||
`options.date` | date | **Deprecated**, use `date`. See [file(name, data [,options])]({{site.baseurl}}/documentation/api_jszip/file_data.html) | ||
`options.compression` | compression | see [file(name, data [,options])]({{site.baseurl}}/documentation/api_jszip/file_data.html) | ||
`options.createSubFolders` | boolean | Automatically create folders in file paths. Defaults to false. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we document this one ? The boolean is here only because we expose the whole options object, it won't be used after the call to file(name, data)
: the sub folders are already created. I think this will only confuse users.
I'd rather have an undocumented flag than documenting one only to document its removal.
The unit tests are green everywhere (nodejs and all the tested browsers), congrats ! |
Sure, I'll take care of that when I get into the office in the morning. Thanks! -----Original Message----- The unit tests are green everywhere (nodejs and all the tested browsers), congrats ! |
And added unit tests for the functionality
Okay, I removed the mention of the createSubFolders option from the zipObject docs, squashed all of the commits into a single commit, and pushed it up to my fork. Should be ready to go! |
add createSubFolders option
Thanks :) |
My pleasure man, thanks for all of your help! |
@@ -22,6 +22,7 @@ name | type | default | description | |||
options.base64 | boolean | false | set to `true` if the data is base64 encoded, `false` for binary. | |||
options.checkCRC32 | boolean | false | set to `true` if the read data should be checked against its CRC32. | |||
options.optimizedBinaryString | boolean | false | set to true if (and only if) the input is a string and has already been prepared with a 0xFF mask. | |||
options.createSubFolders | boolean | false | set to true to create folders in the file path automatically. Leaving it false will result in only virtual folders (i.e. folders that merely represent part of the file path) being created. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, this should have been named 'createFolders' as it's just about creating actual folders in the zip file.
And added unit tests for the functionality
Fixes #154