refactor: compress through minimizer-webpack-plugin's asset generator - #435
alexander-akait wants to merge 8 commits into
Conversation
…lugin Reading an asset, writing one beside it, caching both and running them where they belong is the same work whether the bytes come back smaller or differently encoded, and the minimizer plugin already does it. What stays here is what compression means by it: which algorithm, what it is run with, and the key the compressed file is recorded under. Work in progress: it needs a release carrying that engine, which is not published yet, and three of its own options still have no answer there - deleteOriginalAssets as "keep-source-map" or a function, and filename as a function. Their tests fail.
A .gz is a new file beside the asset it was read from, which is what an `asset` generator writes. Nothing here minifies, and the engine now reads that off the instance rather than from a `minify: false`. Still failing, both for the same reason - the engine takes only a boolean: `deleteOriginalAssets` as "keep-source-map" or as a function, plus the duplicate-asset reporting that rides on it.
The generator deletes the file it read and nothing its related names, so the string maps onto true and a function passes straight through.
Only the compressed asset's own file goes now, so a source map or another plugin's file beside it stays, and keep-source-map is what true does.
Walkthrough
Priority: ⬇️ Low Merge Risk: 🔴 Critical · up to The new delegation cannot be installed from the declared dependency range, so builds and consumers cannot use this change until a compatible engine release and lockfile entry are available. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
test/deleteOriginalAssets.test.jsOops! Something went wrong! :( ESLint: 9.39.5 Error: Error while loading rule 'jest/no-deprecated-functions': Unable to detect Jest version - please ensure jest package is installed, or otherwise set version explicitly Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: b8f2241e-55c7-405a-ad04-84f159fb8d93
📒 Files selected for processing (3)
README.mdpackage.jsonsrc/index.js
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
| "release": "standard-version" | ||
| }, | ||
| "dependencies": { | ||
| "minimizer-webpack-plugin": "^5.11.0", |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Require the engine release before enabling compression delegation.
The latest published minimizer-webpack-plugin version is 5.10.1. The declared ^5.11.0 version cannot resolve, and the generator contract is scheduled for the next release. Installation therefore fails before this implementation can run. (npmjs.com)
package.json#L49-L49: wait for the release containing PR#742, require that version, and update the lockfile.src/index.js#L246-L259: enable this generator configuration only after the required package version is available.
Based on learnings, fix production compatibility before changing failing tests.
📍 Affects 2 files
package.json#L49-L49(this comment)src/index.js#L246-L259
Source: Learnings
| Determines whether the original (uncompressed) assets should be deleted after compression. | ||
|
|
||
| - If set to `true` , all original assets will be deleted. | ||
| - If set to `true` , all original assets will be deleted. Only the compressed asset's own file goes: a source map, or a file another compression plugin wrote beside it, is kept. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Identify the deleted asset correctly.
“Only the compressed asset's own file goes” says that the compressed output is deleted. The implementation deletes only the original asset.
Proposed documentation fix
-- If set to `true` , all original assets will be deleted. Only the compressed asset's own file goes: a source map, or a file another compression plugin wrote beside it, is kept.
+- If set to `true`, only each original asset is deleted. Its source map and files written beside it by other compression plugins are kept.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - If set to `true` , all original assets will be deleted. Only the compressed asset's own file goes: a source map, or a file another compression plugin wrote beside it, is kept. | |
| - If set to `true`, only each original asset is deleted. Its source map and files written beside it by other compression plugins are kept. |
The sentence read as if the compressed file went; it is the original, and only that one.
The engine defaults terser and a .js test for an instance that minifies; this one does not, so it asks for no minimizers and keeps compression's own every-asset default.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/index.js (1)
263-263: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for multiple compression instances.
CompressionPlugin.apply()passesthis.relatedName()to the generator. The deletion test uses one instance, while existing multi-instance tests do not enabledeleteOriginalAssets. Add a test with two algorithms anddeleteOriginalAssets: true, then assert that both compressed outputs remain.Source: Learnings
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: d14ca364-07ba-48fd-99f0-4b3786bc4450
⛔ Files ignored due to path filters (2)
test/__snapshots__/algorithm.test.js.snapis excluded by!**/*.snaptest/__snapshots__/deleteOriginalAssets.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
README.mdsrc/index.jstest/deleteOriginalAssets.test.js
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
| // This plugin compresses what a build emitted and changes none of it. | ||
| minify: [], | ||
| generate: { | ||
| implementation: MinimizerPlugin.compress, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '35,60p' package.json
sed -n '1,100p' package-lock.json
rg -n '"minimizer-webpack-plugin"|npm ci|npm install|package-lock' package.json package-lock.json .github README.md 2>/dev/null | head -160
sed -n '185,275p' src/index.jsRepository: webpack/compression-webpack-plugin
Length of output: 6978
🏁 Script executed:
python3 - <<'PY'
import json, urllib.request
url = "https://registry.npmjs.org/minimizer-webpack-plugin"
with urllib.request.urlopen(url, timeout=20) as response:
data = json.load(response)
print("dist-tags:", json.dumps(data.get("dist-tags"), sort_keys=True))
for version in ["5.11.0", "5.11.1", "5.12.0", "6.0.0"]:
item = data.get("versions", {}).get(version)
if item:
print("\nVERSION", version)
print("package:", json.dumps({
k: item.get(k) for k in ["version", "dependencies", "peerDependencies", "engines", "dist"]
}, sort_keys=True))
print("repository:", item.get("repository"))
PY
printf '\n--- relevant package metadata/source references ---\n'
python3 - <<'PY'
import json, urllib.request, tarfile, io, re
base = "https://registry.npmjs.org/minimizer-webpack-plugin"
with urllib.request.urlopen(base, timeout=20) as response:
data = json.load(response)
for version in ["5.11.0", "5.11.1"]:
item = data.get("versions", {}).get(version)
if not item:
continue
tarball = item["dist"]["tarball"]
with urllib.request.urlopen(tarball, timeout=20) as response:
raw = response.read()
with tarfile.open(fileobj=io.BytesIO(raw), mode="r:gz") as archive:
print("\nVERSION", version)
for member in archive.getmembers():
if member.isfile() and re.search(r"(package\.json|dist/.*\.(js|mjs|cjs|d\.ts)|src/.*\.(js|ts))$", member.name):
text = archive.extractfile(member).read().decode("utf-8", "replace")
if "compress" in text or "generate" in text or member.name.endswith("package.json"):
print("---", member.name)
for i, line in enumerate(text.splitlines(), 1):
if re.search(r"compress|generate|version|peerDependencies|webpack", line):
print(f"{i}:{line[:240]}")
PY
printf '\n--- npm ci mismatch behavior help ---\n'
npm --version
npm ci --help | sed -n '1,80p'Repository: webpack/compression-webpack-plugin
Length of output: 7833
🏁 Script executed:
python3 - <<'PY'
import json, urllib.request
with urllib.request.urlopen("https://registry.npmjs.org/minimizer-webpack-plugin", timeout=20) as r:
d=json.load(r)
print("latest", d.get("dist-tags",{}).get("latest"))
print("matching versions", [v for v in d.get("versions",{}) if v.startswith("5.")][-20:])
for v in sorted(d.get("versions",{})):
if v.startswith("5."):
p=d["versions"][v]
print(v, p.get("dist",{}).get("tarball"))
PYRepository: webpack/compression-webpack-plugin
Length of output: 2458
🏁 Script executed:
set -eu
tmp="$(mktemp)"
trap 'rm -f "$tmp"' EXIT
curl -fsSLk --max-time 20 https://registry.npmjs.org/minimizer-webpack-plugin > "$tmp"
python3 - "$tmp" <<'PY'
import json, sys
data = json.load(open(sys.argv[1]))
versions = data.get("versions", {})
print("dist-tags:", json.dumps(data.get("dist-tags", {}), sort_keys=True))
print("5.x versions:", json.dumps([v for v in versions if v.startswith("5.")]))
for version in sorted(versions):
if version.startswith("5."):
item = versions[version]
print("\nVERSION", version)
print("dist:", json.dumps(item.get("dist", {}), sort_keys=True))
print("repository:", json.dumps(item.get("repository"), sort_keys=True))
PYRepository: webpack/compression-webpack-plugin
Length of output: 6666
🏁 Script executed:
set -eu
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
curl -fsSLk --max-time 30 https://registry.npmjs.org/minimizer-webpack-plugin/-/minimizer-webpack-plugin-5.10.1.tgz -o "$tmp/pkg.tgz"
printf '%s\n' '--- archive members ---'
tar -tzf "$tmp/pkg.tgz"
printf '%s\n' '--- package.json ---'
tar -xOf "$tmp/pkg.tgz" package/package.json
printf '%s\n' '--- source references to compress/generate ---'
tar -xOf "$tmp/pkg.tgz" package/dist/index.js 2>/dev/null | grep -nE 'compress|generate|minify|relatedName|deleteOriginalAssets' || true
tar -xOf "$tmp/pkg.tgz" package/dist/index.d.ts 2>/dev/null | grep -nE 'compress|generate|minify|relatedName|deleteOriginalAssets' || trueRepository: webpack/compression-webpack-plugin
Length of output: 13928
🏁 Script executed:
set -eu
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
curl -fsSLk --max-time 30 https://registry.npmjs.org/minimizer-webpack-plugin/-/minimizer-webpack-plugin-5.10.1.tgz -o "$tmp/pkg.tgz"
tar -xOf "$tmp/pkg.tgz" package/dist/index.js > "$tmp/index.js"
printf '%s\n' '--- exports and implementation ---'
grep -nE '^(class|function)|module\.exports|exports\.|\.compress|\.minify|generateAsset|generator\s*:' "$tmp/index.js" | tail -80
printf '%s\n' '--- generator normalization and asset generation ---'
sed -n '270,305p;900,950p;1050,1150p' "$tmp/index.js"Repository: webpack/compression-webpack-plugin
Length of output: 8829
Pin the unreleased minimizer-webpack-plugin API in both manifests.
The registry publishes minimizer-webpack-plugin only through 5.10.1, so ^5.11.0 cannot resolve. Version 5.10.1 exports TerserPlugin without compress and does not accept the generator descriptor used here. package-lock.json also omits the dependency, so the workflow’s npm ci fails before the build. Pin an API-compatible prerelease or commit, then regenerate package-lock.json.
Deleting an original took everything its related named with it, so a gzip instance destroyed a brotli one's files; the case now fails where that happens.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: dadc12cd-d284-46e7-9381-d7ec89541086
📒 Files selected for processing (1)
test/deleteOriginalAssets.test.js
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.
Without it the case passes where nothing is deleted at all, which is not what it is about.
1 participant
Summary
This plugin reads, writes, caches and schedules assets itself, all of which
minimizer-webpack-pluginnow does for a generator that writes a file beside an emitted one. Compressing is that:src/index.jsdrops from 452 to 266 lines and states the algorithm,threshold,minRatio,filename,deleteOriginalAssetsand therelatedkey, while the engine does the rest.Deleting an original now takes that file alone. A source map, or a file another compression instance wrote beside it, is kept — today
deleteOriginalAssets: truedeletes both through webpack'srelatedcascade, which is how a gzip instance destroys a brotli one's output."keep-source-map"is therefore whattruealready does, and is kept as an alias.Draft: it depends on unreleased engine work (webpack/minimizer-webpack-plugin#742) and on one open question — the engine applies terser and a
.jstestby default, so this wrapper currently minifies the assets it compresses and skips every non-JS one. 46 of 106 tests fail on that, all of them for that reason. Not for review until it is settled.What kind of change does this PR introduce?
refactor.
Did you add tests for your changes?
No new tests; this is covered by the existing suite, which has to go green before this leaves draft. The engine-side behaviour has tests in webpack/minimizer-webpack-plugin#742.
Does this PR introduce a breaking change?
No migration is needed, but one behaviour changes:
deleteOriginalAssets: trueno longer deletes the deleted asset's source map, or any other file named in itsrelatedinfo. Anyone relying on the map going away must delete it themselves.If relevant, what needs to be documented once your changes are merged or what have you already documented?
Documented here: the
deleteOriginalAssetssection of the README says what deletion takes with it and that"keep-source-map"is an alias fortrue.Use of AI
Claude Code was used for the refactor, the investigation behind it and this description. The
relatedcascade was confirmed by running both implementations over the same fixtures — gzip plus brotli withdeleteOriginalAssets: trueemits no.brfiles onmain.Generated by Claude Code
Summary by CodeRabbit
Documentation
Bug Fixes