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.
There was a problem hiding this comment.
You shouldn't update the version / changelog, this will be done at release time.
|
Ah, I'll undo that then |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
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 |
There was a problem hiding this comment.
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! |
There was a problem hiding this comment.
IMHO, this should have been named 'createFolders' as it's just about creating actual folders in the zip file.
3 participants
And added unit tests for the functionality
Fixes #154