search

bower / decompress-zip

Module that decompresses zip files
MIT License
102 stars 76 forks source link

ENOENT bug - Directories not created #38

Closed prust closed 9 years ago

prust commented 9 years ago

@wibblymat: this fixes #29, which @omnidan first reported and @BCooper63 found the root cause of.

I needed a patch, so I did the easy/simple thing, which was to expose the cache on the extractors module and clear it out every time extract() is called. But I don't really think it's the correct solution. I suppose the most robust, correct solution would be to attempt the mkdir every time (and catch the error if it fails due to already being there) b/c the directory could hypothetically be deleted at any time by any other process. But that's probably a bit over-the-top, the more practical and expected behavior would be for the cache to be tied to the decompress-zip instance, as @BCooper63 suggests, instead of global to the module, as it is currently. I didn't implement it this way b/c it would've require a deeper refactoring & I wasn't sure if that's the direction you guys want to go with this and, if so, how exactly you would want to implement it (pass the instance's cache to the extractors module?)

At any rate, this pull request fixes the bug in a minimal way for us, but could cause bugs, for instance if two decompress-zip instances are fired up, one just after the other, the second one, when instantiated, will clear out the global cache and could potentially mess up the first one that is in the middle of running...

prust commented 9 years ago

Also note that I haven't written a regression test for this yet (it shouldn't be too hard -- just run fs.rmdirSync() between calls to extract()).

sindresorhus commented 9 years ago

Thanks for effort, but you're right, this is not the right solution. Many users use Bower concurrently. Let's continue the discussion in #38, but this sounds like a good solution to me:

the more practical and expected behavior would be for the cache to be tied to the decompress-zip instance, as @BCooper63 suggests, instead of global to the module, as it is currently.

Though I'm not familiar with this codebase, so I'll defer to @wibblymat or @sheerun.