Skip to content

Commit

Permalink
build(webpack): fix --zip in Node.js 22 by cloning assets into `Uin…
Browse files Browse the repository at this point in the history
…t8Array`s before zipping (#29177)

Use a copy of the Buffer via `Buffer.from(asset)`, as Zip will *consume*
it, which breaks things if we are compiling for multiple browsers at
once. `Buffer.from` uses the internal pool, so it's superior to `new
Uint8Array` if we don't need to pass it off to a worker thread.

Additionally, in Node.js 22+ a Buffer marked as "Untransferable" (like
ours) can't be passed to a worker, which `AsyncZipDeflate` uses. See:
101arrowz/fflate#227 (comment)

This can probably be simplified to `zipFile.push(Buffer.from(asset),
true);` if the above issue is resolved.

This fix should hopefully unblock
#28368


<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.


## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?


[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29177?quickstart=1)

## **Related issues**

Fixes:

## **Manual testing steps**

1. Go to this page...
2.
3.

## **Screenshots/Recordings**


### **Before**


### **After**


## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

-->
  • Loading branch information
davidmurdoch authored Dec 16, 2024
1 parent 207245f commit 58ca7af
Showing 1 changed file with 17 additions and 18 deletions.
35 changes: 17 additions & 18 deletions development/webpack/utils/plugins/ManifestPlugin/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,6 @@ type Assets = Compilation['assets'];
const NAME = 'ManifestPlugin';
const BROWSER_TEMPLATE_RE = /\[browser\]/gu;

/**
* Clones a Buffer or Uint8Array and returns it
*
* @param data
* @returns
*/
function clone(data: Buffer | Uint8Array): Buffer {
return Buffer.from(data);
}

/**
* Adds the given asset to the zip file
*
Expand All @@ -54,12 +44,23 @@ function addAssetToZip(
zip: Zip,
): void {
const zipFile = compress
? new AsyncZipDeflate(assetName, compressionOptions)
: new ZipPassThrough(assetName);
? // AsyncZipDeflate uses workers
new AsyncZipDeflate(assetName, compressionOptions)
: // ZipPassThrough doesn't use workers
new ZipPassThrough(assetName);
zipFile.mtime = mtime;
zip.add(zipFile);
// use a copy of the Buffer, as Zip will consume it
zipFile.push(asset, true);
// Use a copy of the Buffer via `Buffer.from(asset)`, as Zip will *consume*
// it, which breaks things if we are compiling for multiple browsers at once.
// `Buffer.from` uses the internal pool, so it's superior to `new Uint8Array`
// if we don't need to pass it off to a worker thread.
//
// Additionally, in Node.js 22+ a Buffer marked as "Untransferable" (like
// ours) can't be passed to a worker, which `AsyncZipDeflate` uses.
// See: https://github.com/101arrowz/fflate/issues/227#issuecomment-2540024304
// this can probably be simplified to `zipFile.push(Buffer.from(asset), true);`
// if the above issue is resolved.
zipFile.push(compress ? new Uint8Array(asset) : Buffer.from(asset), true);
}

/**
Expand Down Expand Up @@ -145,7 +146,7 @@ export class ManifestPlugin<Z extends boolean> {
errored = true;
reject(error);
} else {
zipSource.add(new RawSource(clone(data)));
zipSource.add(new RawSource(Buffer.from(data)));
// we've received our final bit of data, return the zipSource
if (final) resolve(zipSource);
}
Expand All @@ -171,9 +172,7 @@ export class ManifestPlugin<Z extends boolean> {
if (excludeExtensions.includes(extName)) continue;

addAssetToZip(
// make a copy of the asset Buffer as Zipping will *consume* it,
// which breaks things if we are compiling for multiple browsers.
clone(asset.buffer()),
asset.buffer(),
assetName,
ManifestPlugin.compressibleFileTypes.has(extName),
compressionOptions,
Expand Down

0 comments on commit 58ca7af

Please sign in to comment.