Skip to content

Conversation

@Hweinstock
Copy link
Contributor

@Hweinstock Hweinstock commented Mar 19, 2025

Problem

This PR solves two very related problems so there are done together in one PR:

  1. ZipUtil uses admZip which not web compatible. Also, the larger issue is that ZipUtil duplicates prepareRepoData in some ways, and it is difficult to align their implementations if they use different libraries.

  2. The ZipStream utility is a wrapper around the zip.js library. However, it imports it via CommonJS require syntax causing us to lose type information. This is dangerous because we are assuming properties and functions exist within the library, and are blind to changes in the underlying library. This makes the ZipStream module difficult and unsafe to work with.

Solution

  • For Problem 1:
    • migrate ZipUtil away from admZip.
    • move ZipUtil tests to live in core package since ZipUtil lives there.
    • add some utility functions to ZipStream + tests for them.
  • For Problem 2:
    • Import zip.js as a module and tell the compiler to ignore it. It looks like this happens because zip.js doesn't support CommonJS, and in general doesn't support Node officially (source: require() of ES modules is not supported. gildas-lormeau/zip.js#362). However, a future commit appears to add this support (on that same issue) so unsure why we still get the error. Experimentally this is working, so its likely Webpack handles this for us.
    • This requires updating some type signatures and also reveals some slight bugs in the implementation.

also addresses: #6566

Verification

  • Used /review on a midsize project and it produced the same results on this and previous release (without changes).

  • Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
  • Your code changes must meet the guidelines in CONTRIBUTING.md.
  • License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@Hweinstock
Copy link
Contributor Author

Hweinstock commented Mar 19, 2025

/runIntegrationTests

@Hweinstock Hweinstock changed the title deps(ziputil): migrate ziputil away from admzip (WIP) deps(ziputil): migrate ziputil to use a web-safe and typed ZipStream (WIP) Mar 20, 2025
@Hweinstock Hweinstock changed the title deps(ziputil): migrate ziputil to use a web-safe and typed ZipStream (WIP) deps(ziputil): migrate ziputil to use a web-safe and typed ZipStream Mar 20, 2025
@Hweinstock Hweinstock marked this pull request as ready for review March 20, 2025 19:20
@Hweinstock Hweinstock requested review from a team as code owners March 20, 2025 19:20
}

public static async unzip(zipBuffer: Buffer): Promise<Entry[]> {
const reader = new ZipReader(new Uint8ArrayReader(new Uint8Array(zipBuffer)))
Copy link
Contributor

Choose a reason for hiding this comment

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

off topic: this doesn't actually "stream" the result, it all goes into memory, right? that's something we need to address eventually.

Copy link
Contributor Author

@Hweinstock Hweinstock Mar 20, 2025

Choose a reason for hiding this comment

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

I noticed this as well. Also, the current logic in zipUtils dumps this to the file system before uploading whereas prepareRepoData uploads the buffer itself.

If we can centralize the uploading logic (also has two completely separate implementations), this streaming change will be much easier.

@Hweinstock
Copy link
Contributor Author

Hweinstock commented Mar 21, 2025

#6566 is apparently still an issue since the fix only lives in /doc, /dev implementations. Need to move the fix to ZipStream.

@Hweinstock Hweinstock requested a review from a team as a code owner March 21, 2025 14:20
@Hweinstock Hweinstock merged commit 4459171 into aws:master Mar 21, 2025
30 of 32 checks passed
@Hweinstock Hweinstock deleted the cleanup/zipUtil branch March 21, 2025 20:02
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.

4 participants