refactor!: move ZipFS and ZipOpenFS into @yarnpkg/libzip
#4853
Conversation
This file contains 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
ZipFS and ZipOpenFS were until now stored alongside the rest of the filesystem utilities. While it made some sense to keep all the FakeFS implementation together, it started to become a little awkward after most of the ZipOpenFS logic was moved inside a generic MountFS, leaving ZipFS / ZipOpenFS more related to an external dependency than they were to the package that contained them. It also made it difficult to build tools leveraging them (such as the `mountMemoryDrive` function mentioned below), because we always had to dance with the `libzip` parameter when constructing instances.
This diff changes that, moving ZipFS and ZipOpenFS into `@yarnpkg/libzip`. The benefit API-wise is that since ZipFS and ZipOpenFS are now store alongside libzip, the libzip instance no longer has to be specified when constructing ZipFS / ZipOpenFS instances (i.e. you can now run `new ZipFS()` instead of `new ZipFS(null, {libzip})`). It should work on both Node.js and browser environments, although on browsers it'll require that the ZipFS / ZipOpenFS instances are only constructed after `getLibzipPromise` returned. On Node.js, the libzip will be lazily loaded.
This diff also exposes a new utility called `mountMemoryDrive`. It's an helper whose purpose is to transparently mount memory drives on disk. While not directly related to zip itself, it uses an in-memory zip instance to store the disk mutations. It would (perhaps?) be more optimized to use a dedicated in-memory FakeFS instance, but since it would require to reimplement a lot of the logic that ZipFS already does I don't foresee doing this job anytime soon, if at all. In the meantime, ZipFS is a fine place to store this tool.
arcanis
changed the title
RDIL
reviewed
Sep 16, 2022
| export function getLibzipSync() { | ||
| if (mod === null) | ||
| mod = makeInterface(createModule()); | ||
| setFactory(() => { |
There was a problem hiding this comment.
Can side effects be avoided here? This will remove the safety to tree-shake this module.
There was a problem hiding this comment.
I don't see a way to avoid it; however I can mark this file as having side effects, so that other files will remain tree-shaken
There was a problem hiding this comment.
Actually the package doesn't list sideEffects at all at the moment, so I'm not sure it's tree-shakable 🤔 I'm not too sure what should be the proper value (do we even have to list the entry point as having side effects, since it'll always be included if anything else is included?), so I won't touch it in this diff
merceyz
requested changes
Sep 16, 2022
merceyz
changed the title
ZipFS and ZipOpenFS into @yarnpkg/libzip
3 tasks
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.
What's the problem this PR addresses?
ZipFS and ZipOpenFS were until now stored alongside the rest of the filesystem utilities. While it made some sense to keep all the FakeFS implementation together, it started to become a little awkward after most of the ZipOpenFS logic was moved inside a generic MountFS, leaving ZipFS / ZipOpenFS more related to an external dependency than they were to the package that contained them. It also made it difficult to build tools leveraging them (such as the
mountMemoryDrivefunction mentioned below), because we always had to dance with thelibzipparameter when constructing instances.How did you fix it?
This diff changes that, moving ZipFS and ZipOpenFS into
@yarnpkg/libzip. The benefit API-wise is that since ZipFS and ZipOpenFS are now store alongside libzip, the libzip instance no longer has to be specified when constructing ZipFS / ZipOpenFS instances (i.e. you can now runnew ZipFS()instead ofnew ZipFS(null, {libzip})). It should work on both Node.js and browser environments, although on browsers it'll require that the ZipFS / ZipOpenFS instances are only constructed aftergetLibzipPromisereturned. On Node.js, the libzip will be lazily loaded.This diff also exposes a new utility called
mountMemoryDrive. It's an helper whose purpose is to transparently mount memory drives on disk. While not directly related to zip itself, it uses an in-memory zip instance to store the disk mutations. It would (perhaps?) be more optimized to use a dedicated in-memory FakeFS instance, but since it would require to reimplement a lot of the logic that ZipFS already does I don't foresee doing this job anytime soon, if at all. In the meantime, ZipFS is a fine place to store this tool.Checklist