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

Add createSubFolders option, default to true #30

Merged
merged 1 commit into from Jul 25, 2014

Conversation

isochronous
Copy link
Contributor

I've got a PR (Stuk/jszip#157) in for JSZip to add a createSubFolders option that will fix #29. This pull request just sends that option (defaulting to true if not provided) along to JSZip in each call to .file.

@isochronous
Copy link
Contributor Author

Well, the Travis CI build is failing because the npm dependency for jszip was set to ^2.3.1, which won't exist until the JSZip devs publish the new version. Either I can revert the jszip dependency to 2.3.0 (as the changes I made will be benign in old versions) and get the Travis CI build to pass, and then update the version number again after JSZip publishes, or I can leave it as it is, in which case the Travis build will start working again as soon as 2.3.1 comes out. Let me know which you'd prefer.

@sindresorhus
Copy link
Owner

Why would there need to be an option for this? Seems like it can just be on by default unless I'm missing something.

@isochronous
Copy link
Contributor Author

It is on by default. However, there are apparently use cases in JSZip where the behavior is undesirable, which is why I left a way for users to disable it if they need to.

@sindresorhus
Copy link
Owner

Use-cases like?

@isochronous
Copy link
Contributor Author

Like this: Stuk/jszip#130

My pull request to JSZip has been accepted and merged into master, but I'm not sure when he'll re-publish the npm package.

@isochronous
Copy link
Contributor Author

He's saying he'll re-publish the package later today, but he wants to change the name of the flag first, so let me update the code in this pull request real quick.

@sindresorhus
Copy link
Owner

Yeah, I don't really care about that usecase. Just enable it by default without an option.

@isochronous
Copy link
Contributor Author

You da boss

@isochronous
Copy link
Contributor Author

Okay, done - I also rebased all of the commits into one commit for easier merging and tracking

@@ -1,6 +1,6 @@
{
"name": "gulp-zip",
"version": "0.4.0",
"version": "0.5.0",
Copy link
Owner

Choose a reason for hiding this comment

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

don't bump other peoples versions. i have a bump script i run on publish time ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, I should have known better =)

@isochronous
Copy link
Contributor Author

Okay, done

@isochronous
Copy link
Contributor Author

You were right about the JSZip version number, so I've updated the package.json file to require ^2.4.0 instead.

@sindresorhus
Copy link
Owner

@isochronous see #31

@isochronous
Copy link
Contributor Author

Well, it's nice that it works without adding the option, but since the option is being added (stuk is going to publish it to npm as soon as he gets to a computer) it seems like that would be unnecessary complexity for something that can be addressed by just sending an option. But it's your call man, like I said before, you da boss.

@isochronous
Copy link
Contributor Author

2.4.0 has been released

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.

By sending {createFolders: true} as an option to JSZip's `.file` method,
true folders are created (with D attributes, method properties, etc) rather
than just virtual representations of folders that are part of the files'
paths.

This requires updating the required version of JSZip to 2.4.0.
sindresorhus added a commit that referenced this pull request Jul 25, 2014
Add `createSubFolders` option, default to `true`
@sindresorhus sindresorhus merged commit 5165100 into sindresorhus:master Jul 25, 2014
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.

D attribute is missing from zipped directories
2 participants