empty directories are not created

[fredericrous]

I download package electron-v0.30.2-darwin-x64.zip from electron repository at https://github.com/atom/electron/releases/tag/v0.28.1

I write the following and execute it with node:

var extract = require('extract-zip')
    extract("electron-v0.30.2-darwin-x64.zip", {dir: "."}, function(err) {
        console.log(err)
    })

It extract these files

➜  .electron  ls ~/.node/lib/node_modules/electron-prebuilt/dist/Electron.app/Contents/Resources 
atom.asar   atom.icns   default_app

when it should have extracted these files:

➜  .electron  ls Electron.app/Contents/Resources/
am.lproj     bn.lproj     default_app  es_419.lproj fr.lproj     hu.lproj     ko.lproj     ms.lproj     pt_PT.lproj  sr.lproj     th.lproj     zh_TW.lproj
ar.lproj     ca.lproj     el.lproj     et.lproj     gu.lproj     id.lproj     lt.lproj     nb.lproj     ro.lproj     sv.lproj     tr.lproj
atom.asar    cs.lproj     en.lproj     fa.lproj     he.lproj     it.lproj     lv.lproj     nl.lproj     ru.lproj     sw.lproj     uk.lproj
atom.icns    da.lproj     en_GB.lproj  fi.lproj     hi.lproj     ja.lproj     ml.lproj     pl.lproj     sk.lproj     ta.lproj     vi.lproj
bg.lproj     de.lproj     es.lproj     fil.lproj    hr.lproj     kn.lproj     mr.lproj     pt_BR.lproj  sl.lproj     te.lproj     zh_CN.lproj

[max-mapper]

here's the line causing the issue:

https://github.com/maxogden/extract-zip/blob/master/index.js#L28-L31

basically in extract-zip I ignore 'directory' entries, I wait until there's a file and create the directory then: https://github.com/maxogden/extract-zip/blob/master/index.js#L89

apparently all the .lproj files are empty directories which is why none of them get created. it seems like we should change the behavior of extract-zip to support empty directories, though

[smebberson]

@maxogden, what sort of help are you looking for here?

[max-mapper]

@smebberson hi, thanks for asking. Here's what I think a PR would need:

• change this behavior to instead create an empty folder https://github.com/maxogden/extract-zip/blob/master/index.js#L28-L31. As far as I can tell directories always end with /
• you'll need to modify https://github.com/maxogden/extract-zip/blob/master/index.js#L47 to have a 'create empty folder' code path
• add a basic test
• make this behavior optional, enabled by default but possible to disable

If you have any specific code questions let me know

[smebberson]

@maxogden, okay, doesn't seem too difficult. I just had a look and there doesn't seem to be what I'd call a test framework in place... what are you thinking a 'basic test' would look like?

[max-mapper]

@smebberson oh sorry about that. I usually use require('tape') for assertions, but skimped on the tests so far in here. For now a basic test should be a fixture zip that has empty dirs in it, and a js file that extracts it and checks to make sure the empty dir was created. I'll convert the 'tests' over to tape in the future (or feel free if you want to do it).

I also remembered another aspect of this which might be more difficult. Currently it does mkdirp for every entry's directory https://github.com/maxogden/extract-zip/blob/master/index.js#L89. I'm not sure if theres a better approach to calling mkdirp. We could call it for every empty directory, and every entry's parent directory, but that would result in calling mkdirp multiple times for the same directories.

It would work, it would just be somewhat inefficient. One thing we could do is have an in-memory cache of folders that have already had mkdirp called on them, so you know which ones you can skip. Another might be to call fs.exists on the folder first, as its cheaper than mkdirp (I think).

[smebberson]

I think a simple in-memory cache would be best, much quicker than fs.exists multiple times for the same directory.

Reading through the code, I think it would be best to push both directories and files into the q queue, and handle either directory creation, file creation (including directory creation) and symlink all within the extractEntry function.

If you're cool with that, I'll give it a shot, and I'll write a test, and rewrite the current test using tape.

[smebberson]

@maxogden, I've started on this with PR #9. I've started with the test harness.

[smebberson]

@maxogden, PR #9 is now complete and empty directories are created successfully. Please review the PR with a merge and npm publish in mind?

Because the async.queue had a concurrency of one, I was able to implement what was required with out any in-memory cache being required. As directories are encountered first, I just created them first.

[max-mapper]

fixed in 1.1.0