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

Fix 691: unzip files using a stream to reduce RAM load #756

Merged
merged 1 commit into from
Dec 4, 2023

Conversation

davidmurray
Copy link
Collaborator

@davidmurray davidmurray commented Nov 9, 2023

In Transition, we zip and unzip files for GTFS exporting and importing.
I could not find a package that allowed unzipping using a stream but also allow zipping files. This PR uses https://github.com/antelle/node-stream-zip, which only allows unzipping. We must therefore keep our current dependency on JSZip.

The options are as follows:

  1. Accept this PR which unzips using a stream, but adds an extra package dependency
  2. Spend time writing our own zip/unzip code (or find an existing library to extend/improve)
  3. Drop both packages (JSZip and node-stream-zip) and instead ask users to install the unix zip utility on their workstations (it is preinstalled on macOS and probably most linux distros anyway)

@greenscientist
Copy link
Collaborator

I'm fine with this option 1. Maintaining our own code or having to rely on a system zip, which might be different depending on the OSes all sounds more complicated.

Copy link
Collaborator

@greenscientist greenscientist left a comment

Choose a reason for hiding this comment

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

This looks good to me.

(will need a rebase to re-ruin the test)

Copy link
Collaborator

@tahini tahini left a comment

Choose a reason for hiding this comment

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

Please rebase this PR for the CI to pass, but looks good

This does it using a stream rather than loading all files into RAM at once. Fixes chairemobilite#691
@tahini tahini merged commit 80b6eb5 into chairemobilite:main Dec 4, 2023
4 checks passed
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.

3 participants