feat: an empty minify list, and deleting an original alone - #743
Conversation
The asset it read goes alone, so a source map or another generator's file beside it stays, and a function decides per asset which of them to delete.
For an instance whose whole job is its generate. A list with none in it needs no guard where a second kind of value would, which is why false stays out.
An instance whose whole job is its generate rewrote no bytes, so the bundle kept its own name; and a file written under the original's name has replaced it, so recording related there wrote the source back over it.
…yped as written The public type said one function where the schema has taken a list or a descriptor since 5.10, and a filename function is what webpack's own getPath takes.
The two shapes a compression instance is written in, now that an empty list of minimizers says the second one.
|
| Name | Type |
|---|---|
| minimizer-webpack-plugin | Minor |
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #743 +/- ##
==========================================
+ Coverage 97.73% 97.75% +0.01%
==========================================
Files 4 4
Lines 1545 1557 +12
Branches 574 582 +8
==========================================
+ Hits 1510 1522 +12
Misses 35 35 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. WalkthroughThe plugin now accepts Priority: ⬇️ Low 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
src/index.jsESLint failed to execute (timeout). test/generate-option.test.jsESLint skipped: the matched ESLint configuration already failed (timeout). 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.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · Do not salt embedded-source hashes when no minimizers run. · index.js:2436-2439
src/index.js:2436-2439
🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not salt embedded-source hashes when no minimizers run.
With
minify: [],renderEmbeddedSourcereturns the original source because no minimizer matches. The registeredembeddedSourceHashtap still adds"TerserPlugin"anddatato the module hash. Webpack can therefore change content hashes and derived filenames even though this plugin does no minification, contrary to the documented no-hash-change behavior.Apply the same zero-minimizer guard to
embeddedSourceHash, or do not register the embedded-source hooks whenthis.minimizers().length === 0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 4f028e98-306d-4bd5-80f4-1b1a39b6aa6d
⛔ Files ignored due to path filters (1)
test/__snapshots__/validate-options.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
.changeset/generator-compression-options.mdREADME.mdsrc/index.jssrc/options.jsontest/generate-option.test.jstest/validate-options.test.jstypes/index.d.ts
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
The tap varies a module's hash on the minimizers, so an instance with none of them renamed files it never rewrote — the same defect the chunk hash had.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · Delete only the captured original asset. · index.js:1785
src/index.js:1785
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDelete only the captured original asset.
generateAssetsruns all generators concurrently. A same-name generator can replacenamebefore a differently named generator reaches this branch. The currentgetAsset(name)check then overwrites and deletes that replacement.Compare the current source with the captured
sourcebefore updating and deleting:const current = compilation.getAsset(name); if ( generatedName !== name && current && current.source === source ) {Add a regression test with one same-name generator and one differently named generator. Assert that the same-name replacement remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 5612b384-5047-4e34-b03f-e43a6a3b224b
📒 Files selected for processing (2)
src/index.jstest/embedded-source.test.js
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
The generators of a pass run together, so another may have written over the original first — deleting then destroys that file instead.
1 participant
Summary
The work that missed #742 — it was merged at
214b37e, and these five commits landed on the branch after that.Deleting an original now takes that file and nothing else. webpack deletes whatever an asset's
relatednames along with it, sodeleteOriginalAssetstook the source map, and the file a second generator wrote beside the same original, with it — which is how a gzip instance destroys a brotli one's output (webpack/compression-webpack-plugin#245, webpack/compression-webpack-plugin#389). It also takes a function now, asked per asset.minify: []is nothing to minify, for an instance whose whole job is itsgenerate. A list with none in it needs no guard anywhere the minimizers are run — each pass simply does nothing — while a second kind of value would, which is whyfalsestays out. Such an instance salts no chunk hash either: it rewrites nothing, so the bundle keeps the name it would have had without it.Two more from the same work. A file written under the original's own name has replaced it, so
relatedNameis not recorded there — doing so wrote the source back over what was just generated. Andfilenametakes a function, which is what webpack's owngetPathaccepts and whatcompression-webpack-pluginhas always documented;minify's public type now says what the schema has taken since 5.10 — a list or a descriptor, not one function.What kind of change does this PR introduce?
feat.
Did you add tests for your changes?
Yes, nine in
test/generate-option.test.js: the source map and a sibling generator's file surviving a delete, a function deciding which to delete, a file written under the original's name kept and pointing nowhere, afilenamefunction, an emptyminifyminifying nothing and renaming nothing, and the validation case forminify: []. 719 green.Does this PR introduce a breaking change?
No. One behaviour narrows:
deleteOriginalAssets: trueno longer deletes files named in the deleted asset'srelatedinfo. That was the bug above.If relevant, what needs to be documented once your changes are merged or what have you already documented?
Documented here:
minify's empty list, the generator'sdeleteOriginalAssetsfunction form and what deletion takes, thefilenamefunction, in-place generation, and a new "Compressing with minifying, and without" section undercompress.Use of AI
Claude Code was used for the implementation, tests and this description. The
relatedcascade was found by running bothcompression-webpack-pluginimplementations over the same fixtures: gzip plus brotli withdeleteOriginalAssets: trueemits no.brfiles at all today.Generated by Claude Code
Summary by CodeRabbit
minify: []can skip minification while still running asset generation.