feat: let a generator say what is worth writing, and minify nothing - #742
Conversation
Compressing an asset is not always worth a second file, so an `asset` generator now takes the three things that decide it. `threshold` skips an asset too small to bother with, before the generator is even asked. `minRatio` drops a result that is not enough smaller than what it read, since a file that saves nothing still costs a request. `relatedName` records the new file under that key on the one it came from — which is how a server asked for the original finds it — and declines an asset already carrying it. `minify` takes `false`, for an instance whose whole job is its generator. That also settles what `test` defaults to: the `.js` default belongs to minifying, and applying it with nothing to minify hid every image from the generator, so it is not applied then. Two more that follow from the same work. The generated file inherits the original's `immutable` only where the name it was given still derives from the original's, since that is what carried the hash the promise rests on. And the generator tap asks for `additionalAssets`, reading what the hook hands it rather than the whole compilation, so a file another plugin emits late still gets the one that belongs beside it.
🦋 Changeset detectedLatest commit: 214b37e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
`minify: false` stopped the minimizing but not the tap that varies every chunk's hash on what the minimizers are — so adding a generator-only instance renamed every file a user serves, which is the one thing a plugin that minifies nothing has no business doing. The salt and the embedded-source hooks now go up only where something minifies. Found by rewiring `compression-webpack-plugin` onto this: its suite failed on a `[fullhash]` that moved, and the hash a build is named by is not something to notice from a diff.
`false` joins the list the schema renders when `minify` is given something else, so the message that lists it moved. The Node 20+ rows run the whole suite with snapshots enforced — the older rows pass `-u` and would never have caught it, which is how it reached CI.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #742 +/- ##
==========================================
+ Coverage 97.66% 97.73% +0.07%
==========================================
Files 4 4
Lines 1497 1545 +48
Branches 545 580 +35
==========================================
+ Hits 1462 1510 +48
Misses 35 35 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Patch coverage named both, and both are worth saying out loud rather than reaching by accident. A generated file stops claiming `immutable` when the name it was given carries none of the original's — the promise rested on a hash that is no longer in the name — and the three compression fields are refused on an `import` generator, which writes no second file for them to describe.
Compressing an asset is minifying it into a second file, so it belongs in `minify` rather than in a generator: a `minify` descriptor naming a `filename` writes its result there instead of over the asset, with `threshold`, `minRatio`, `relatedName` and `deleteOriginalAssets` deciding whether the file is worth writing at all. Nothing has to say `minify: false` any more, so it no longer takes it. The default minimizer is what an instance given nothing else to do gets, and an instance configured to generate reads whatever its generator takes — the `.js` default belongs to that minifier, not to the plugin. One that only writes beside replaces no asset, so it no longer salts the chunk hash, and it runs where a generator does rather than in the pass that would have replaced its asset.
What the original's info says of its hashes, its module and where its source came from is true of that file and not of the one written beside it, so the new file starts from nothing rather than from a copy. `immutable` is the exception, and only where the name it was given still derives from the original's: that is what carried the hash the promise rests on. `flatMap` went with it — it is newer than the Node this plugin still runs on, and reaching it failed every build that wrote beside an asset.
A file written beside an asset is a new file, not a rewritten one, and the engine already had a place for that. Putting it under `minify` meant excluding those entries from the in-place pass, from the stage grouping and from the chunk-hash salt, giving five options that mean nothing to a real minimizer, and routing them through the generator's own code anyway. What that work was for is kept. `minify` still does not take `false`: the JavaScript minifier is the default only for an instance given nothing else to do, so one configured to generate minifies nothing without having to say so. And a generated file still inherits nothing from the asset it was read from beyond a conditional `immutable`.
…or does Configuring a generator is not a reason to turn the JavaScript minifier off, nor to drop the `.js` default a user's `test` overrides. `terserMinify` declines anything that is not a `.js` file by name through its own `filter`, so an instance written for images is unharmed by a default that names JavaScript. The chunk-hash salt goes back to being unconditional with it: something always rewrites an asset now, so the salt is always earned.
The salt makes a chunk's name vary with what its minimizer is and what it is run with, which is only true of a chunk one of them rewrites. Where `test` names something else, or every minimizer's own filter declines the name, the chunk is left alone and salting it renamed a file nothing here had touched. The name is read off the template while the hash inside it is still being computed, so every hash placeholder stands for one character: what is asked of the name is its path and extension, which no hash carries. A template a function names is unknowable, and salts as before.
…ot be reached A file a function names cannot be read ahead of being called, so the salt stands rather than being skipped on a guess — the one case the guard has to get right in the safe direction, and now a test. The empty-string fallbacks behind each placeholder went with it: a chunk carrying neither a name nor an id is not a chunk, and what those arms guarded against was a name this only reads a path and an extension from.
Its only caller was the guard that asked whether anything minified at all, and that question has no answer to give now that something always does. Its own description still said an empty list was `minify: false`, which is no longer a thing to say.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe plugin adds Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to A supported asset-generator configuration can discard its generated output or remove it entirely, so this should be fixed before merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · Expose the asset-generator descriptor in BasePluginOptions.generate. · index.d.ts:666
types/index.d.ts:666
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winExpose the asset-generator descriptor in
BasePluginOptions.generate.
MinimizerImplementation<EXPECTED_ANY>is a callable function type (BasicMinimizerImplementation<T> & MinimizeFunctionHelpers). It does not accept object descriptors. TypeScript users therefore cannot configuretype,filename,threshold,minRatio, orrelatedName.Add and export a public descriptor type for the object form, then include it in the
generateunion withMinimizerImplementation<EXPECTED_ANY>.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 79223c31-df37-470e-a2f7-ad3ea01f5a7d
⛔ Files ignored due to path filters (2)
test/__snapshots__/test-option.test.js.snapis excluded by!**/*.snaptest/__snapshots__/validate-options.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
.changeset/generator-compression-options.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.
Deleting an asset takes everything its `related` names with it, so recording the generated file there and then deleting the original deleted the file just written — `relatedName` and `deleteOriginalAssets` together emitted nothing at all. The one is now recorded only where the other leaves the original standing, which is what `compression-webpack-plugin` has always done. A generated name that already existed was written over and then returned from, before either of those was reached, so a rebuild owed what a first build paid. The chunk-hash guard asked a `filter` for its answer while handing it an empty object for the asset info it reads, which could decline a chunk it then took: only `test`/`include`/`exclude` decide there, being the part that reads a name. And the descriptor an `asset` generator is written as had no type, so none of `type`, `filename`, `threshold`, `minRatio` or `relatedName` could be reached from TypeScript at all; `minify` no longer offers the `false` the schema rejects.
Reads the taps the plugin left on processAssets rather than only the order its work ran in, so collapsing several stages into one hook fails here.
Two taps where a minimizer and a generator both ask for it, since a generator reads back what a minimizer wrote and cannot run in the same pass.
`additionalAssets` hands each pass whatever was emitted after it ran, its own generated files included, so a broad `test` had the minimizers reading back a compressed one.
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 · Replace metadata when updating an existing generated asset. · index.js:1751-1752
src/index.js:1751-1752
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReplace metadata when updating an existing generated asset.
Webpack v5.109.2 merges object-form metadata with the existing asset info. A pre-existing
immutable: truevalue therefore survives when fixed-name generation passesgeneratedInfo, which omitsimmutable. Consumers may treat changing output as immutable and serve stale content.A callback stores its return value as the new asset info. Return
generatedInfoto remove stale metadata without losing metadata intended for the generated asset.Proposed fix
if (compilation.getAsset(generatedName)) { - compilation.updateAsset(generatedName, generatedSource, generatedInfo); + compilation.updateAsset(generatedName, generatedSource, () => generatedInfo); } else { compilation.emitAsset(generatedName, generatedSource, generatedInfo); }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: d614a483-0a79-4025-9480-da7d6d39222e
📒 Files selected for processing (3)
src/index.jstest/generate-option.test.jstypes/index.d.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- types/index.d.ts
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.
webpack merges an info object into the old one, so writing over an existing name kept its immutable and sourceFilename; a function replaces instead.
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 · Preserve the generated source when recording relatedName. · index.js:1774-1776
src/index.js:1774-1776
🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the generated source when recording
relatedName.When neither
generator.filenamenoroutput.filenameis set,generatedNamefalls back toname. The precedingupdateAssettherefore writesgeneratedSourceto the original asset, but this branch replaces it with the originalsource. IfdeleteOriginalAssetsis set, the deletion branch also deletes that same-name generated asset.Update only the asset info when the names differ. Preserve existing
relatedentries.Proposed fix
- if (generator.deleteOriginalAssets) { + if (generator.deleteOriginalAssets && generatedName !== name) { // Deleting an asset takes everything its `related` names with it, so // recording this file there first would delete the file just written. if (compilation.getAsset(name)) { compilation.deleteAsset(name); } return; } - if (generator.relatedName) { - compilation.updateAsset(name, source, { - related: { [generator.relatedName]: generatedName }, - }); + if (generator.relatedName && generatedName !== name) { + compilation.updateAsset( + name, + (currentSource) => currentSource, + (currentInfo) => ({ + ...currentInfo, + related: { + ...currentInfo.related, + [generator.relatedName]: generatedName, + }, + }), + ); }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: ff8599b5-df12-4443-adee-5eb23e4444f6
📒 Files selected for processing (2)
src/index.jstest/generate-option.test.js
Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review.
A generator re-encoding an asset in place names it what it was called, and deleting the original then deleted the file just written, emitting nothing.
1 participant
Summary
Writing a file beside another is not always worth it, so an
assetgenerator now takes the three things that decide.thresholdskips an asset too small to bother with, before the generator is even asked.minRatiodrops a result that is not enough smaller than what it read, since a file that saves nothing still costs a request.relatedNamerecords the new file under that key on the one it came from — which is how a server asked for the original finds it — and declines an asset already carrying it.minifytakesfalse, for an instance whose whole job is its generator. That also settles whattestdefaults to: the.jsdefault belongs to minifying, and applying it with nothing to minify hid every image from the generator, which was the point of the instance. Nothing minifying is represented as an empty list of minimizers rather than a missing one, so every pass over them simply does nothing and no call site needs a guard.Two more follow from the same work. The generated file inherits the original's
immutableonly where the name it was given still derives from the original's, since that is what carried the hash the promise rests on. And the generator tap asks foradditionalAssets, reading what the hook hands it rather than the whole compilation, so a file another plugin emits late still gets the one that belongs beside it.Together with
getStageandgetAssetFlagthis is everythingcompression-webpack-pluginneeds from a generator, which is what it is for.What kind of change does this PR introduce?
feat.
Did you add tests for your changes?
Yes, ten cases in
test/generate-option.test.js:thresholdabove and below,minRatiokeeping and dropping,relatedNamerecorded and declined,minify: falsewith and without atest, that nothing is minified, and an asset emitted atPROCESS_ASSETS_STAGE_REPORTstill getting its twin. 667 green.Does this PR introduce a breaking change?
No. All three generator fields and
minify: falseare opt-in, andtestkeeps its.jsdefault wherever anything minifies. Theimmutablechange is a narrowing — a generated file that inherited the flag without carrying the original's name no longer does, which was a wrong promise.If relevant, what needs to be documented once your changes are merged or what have you already documented?
The three generator fields and
minify: falsewant a line each in thegenerateandminifysections of the README.Use of AI
Claude Code was used for the implementation, tests and this description. The
additionalAssetscase was checked by reverting the option and confirming the test fails — worth noting because the first attempt at that check patched the minimizer tap instead, the option text being identical in both.🤖 Generated with Claude Code
https://claude.ai/code/session_01QzYJtFXxR2SJRGUWehnRGu
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Validation
minify: falseis no longer accepted.