add createSubFolders option #157

isochronous · 2014-07-21T14:49:40Z

And added unit tests for the functionality
Fixes #154

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.

dduponchel · 2014-07-21T20:22:32Z

CHANGES.md

@@ -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))


You shouldn't update the version / changelog, this will be done at release time.

isochronous · 2014-07-21T20:23:49Z

Ah, I'll undo that then

dduponchel · 2014-07-21T20:23:57Z

lib/object.js

+    var dataType = utils.getTypeOf(data),
+        parent;
+
+    if (o && o.createSubFolders && (parent = parentFolder(name))) {


Could you move this test just after the call to prepareFileAttrs ? That way, you won't have to check if o is here.

isochronous · 2014-07-21T20:25:11Z

Sure thing

isochronous · 2014-07-21T20:28:42Z

Okay, I moved that check and undid the version updates

dduponchel · 2014-07-21T20:35:40Z

lib/object.js

-            });
-            for (var i = 0; i < kids.length; i++) {
-                delete this.files[kids[i].name];
+        if (file) {


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.

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.

dduponchel · 2014-07-21T20:45:21Z

The createSubFolders in the JSZip constructor / load() method is a good idea, but you need to add a createSubFolders : options.createSubFolders in lib/load.js :) (and a line in documentation/api_jszip/load.md)

isochronous · 2014-07-21T20:53:50Z

Will do

isochronous · 2014-07-21T21:00:41Z

Okay, the load script and documentation have been updated, but I'm still not really sure what to do about that remove method.

dduponchel · 2014-07-21T21:02:14Z

The last error comes from folderAdd : this function calls fileAdd to create the entry (with the directory flag) but the initial createSubFolders setting is lost. You'll need to pass the value to folderAdd and give it to fileAdd. With this, the sub folders will be created all the way down.

isochronous · 2014-07-21T21:07:13Z

Okay, done. I've got to go eat lunch but I'll respond to any other comments you make when I'm done.

dduponchel · 2014-07-21T21:08:34Z

I'm still not really sure what to do about that remove method.

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.

dduponchel · 2014-07-21T21:35:38Z

I've got to go eat lunch

On my side, I'll go to bed. See you tomorrow !

isochronous · 2014-07-21T23:31:18Z

There we go

isochronous · 2014-07-22T14:51:14Z

Finally, all tests passing

dduponchel · 2014-07-22T22:51:43Z

documentation/api_zipobject.md

+`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.


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.

dduponchel · 2014-07-22T22:58:25Z

The unit tests are green everywhere (nodejs and all the tested browsers), congrats !
After the resolution of the last documentation issue, could you rebase/squash your commits into a single commit ?

isochronous · 2014-07-23T00:30:42Z

Sure, I'll take care of that when I get into the office in the morning. Thanks!

-----Original Message-----
From: "David Duponchel" notifications@github.com
Sent: ‎7/‎22/‎2014 6:58 PM
To: "Stuk/jszip" jszip@noreply.github.com
Cc: "Jeremy McLeod" isochronous@gmail.com
Subject: Re: [jszip] add createSubFolders option (#157)

The unit tests are green everywhere (nodejs and all the tested browsers), congrats !
After the resolution of the last documentation issue, could you rebase/squash your commits into a single commit ?
—
Reply to this email directly or view it on GitHub.

And added unit tests for the functionality

isochronous · 2014-07-23T17:01:45Z

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

dduponchel · 2014-07-23T17:17:35Z

Thanks :)

isochronous · 2014-07-23T17:18:05Z

My pleasure man, thanks for all of your help!

sindresorhus · 2014-07-23T19:05:27Z

documentation/api_jszip/load.md

@@ -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.


IMHO, this should have been named 'createFolders' as it's just about creating actual folders in the zip file.

isochronous mentioned this pull request Jul 21, 2014

Add createSubFolders option, default to true sindresorhus/gulp-zip#30

Merged

dduponchel reviewed Jul 21, 2014
View reviewed changes

dduponchel reviewed Jul 22, 2014
View reviewed changes

add createSubFolders option

f0de745

And added unit tests for the functionality

dduponchel added a commit that referenced this pull request Jul 23, 2014

Merge pull request #157 from isochronous/master

70fae4b

add createSubFolders option

dduponchel merged commit 70fae4b into Stuk:master Jul 23, 2014

sindresorhus reviewed Jul 23, 2014
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add createSubFolders option #157

add createSubFolders option #157

isochronous commented Jul 21, 2014

dduponchel Jul 21, 2014

isochronous commented Jul 21, 2014

dduponchel Jul 21, 2014

isochronous commented Jul 21, 2014

isochronous commented Jul 21, 2014

dduponchel Jul 21, 2014

isochronous Jul 21, 2014

dduponchel commented Jul 21, 2014

isochronous commented Jul 21, 2014

isochronous commented Jul 21, 2014

dduponchel commented Jul 21, 2014

isochronous commented Jul 21, 2014

dduponchel commented Jul 21, 2014

dduponchel commented Jul 21, 2014

isochronous commented Jul 21, 2014

isochronous commented Jul 22, 2014

dduponchel Jul 22, 2014

dduponchel commented Jul 22, 2014

isochronous commented Jul 23, 2014

isochronous commented Jul 23, 2014

dduponchel commented Jul 23, 2014

isochronous commented Jul 23, 2014

sindresorhus Jul 23, 2014

add createSubFolders option #157

add createSubFolders option #157

Conversation

isochronous commented Jul 21, 2014

dduponchel Jul 21, 2014

Choose a reason for hiding this comment

isochronous commented Jul 21, 2014

dduponchel Jul 21, 2014

Choose a reason for hiding this comment

isochronous commented Jul 21, 2014

isochronous commented Jul 21, 2014

dduponchel Jul 21, 2014

Choose a reason for hiding this comment

isochronous Jul 21, 2014

Choose a reason for hiding this comment

dduponchel commented Jul 21, 2014

isochronous commented Jul 21, 2014

isochronous commented Jul 21, 2014

dduponchel commented Jul 21, 2014

isochronous commented Jul 21, 2014

dduponchel commented Jul 21, 2014

dduponchel commented Jul 21, 2014

isochronous commented Jul 21, 2014

isochronous commented Jul 22, 2014

dduponchel Jul 22, 2014

Choose a reason for hiding this comment

dduponchel commented Jul 22, 2014

isochronous commented Jul 23, 2014

isochronous commented Jul 23, 2014

dduponchel commented Jul 23, 2014

isochronous commented Jul 23, 2014

sindresorhus Jul 23, 2014

Choose a reason for hiding this comment