fix: wait for the zip module to load before traversing in folders:download --zip - #719
Closed
dgilman-perplexity wants to merge 1 commit into
Closed
Conversation
Since the archiver 8 upgrade, _setupZip() awaits a dynamic import before assigning this.zip, but run() started folder traversal without waiting for it. When the first folder listing returned before the module loaded, items took the non-zip path, the archive was never finalized and the .zip was left empty (or the command hung). Load the module up front and keep _setupZip() synchronous so this.zip exists before the first item is processed. Add a regression test that asserts the archive is initialized before _getItems() runs, and verify the entries of the produced archive in the existing --zip test.
dgilman-perplexity
marked this pull request as ready for review
September 10, 2026 16:35
Coverage Report for CI Build 34503039201Coverage at 84.916% (no base build to compare)Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats💛 - Coveralls |
Contributor
|
Thanks for the fix, @dgilman-perplexity! Closing this in favor of #721 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #718
What
folders:download --zipregressed in v4.10.0: the archive is left empty (or the command hangs) and the files are written loose to the destination. #686 switched to the ESM-onlyarchiver8 and made_setupZip()asynctoawait import('archiver'), butrun()does not wait for it, sothis.zipis stillundefinedwhen the first items are processed and they take the non-zip code path.This change:
_loadZipArchive(), awaited inrun()before traversal starts, so the archiver module is loaded up front;_setupZip()synchronous again, sothis.zipis assigned before_getItems()yields the first item (the returned promise still resolves when the output stream closes, as before).Awaiting
_setupZip()itself would deadlock, since its promise only resolves afterfinalize().Tests
should initialize the zip archive before traversing the folder: stubs_getItems()and assertsthis.zipis already set when it is first called. Fails onmain(first--zipdownload in the process races the dynamic import), passes with this change.--ziptest now verifies the archive: it lists the central-directory entries (small helper, no new dependency) and checks that nothing was written next to the.zip. This resolves the 2018@TODOthere.npm test: 1191 passing, 9 pending.eslintandprettierclean on the touched files.Reproduced manually on Node 20.20.1 with a mock API: 4.10.0 gives a 0-byte zip plus loose files 5/5; with this patch applied to 4.10.0, 5/5 valid archives.
box files:zipis unaffected.