feat: load minimizers by module path in workers - #744
Conversation
The default minimizer is named by module path, the minimizer loop reads the slots this branch introduced, and both hash guards ask the slots how many there are.
Plus the case the test plan left open: one function among several paths leaves the whole task on the source path.
A required minimizer is handed the payload as it is, so an extractComments callback or a function in a minimizer's own options died as could not be cloned; generators stay callable-only, which is all the schema takes.
🦋 Changeset detectedLatest commit: 86c83d3 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 |
|
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:
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. WalkthroughThe plugin now accepts minimizer functions, module-path strings, and Priority: ➖ Normal 🚥 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/implementation.jsESLint failed to execute (timeout). src/index.jsESLint skipped: the matched ESLint configuration already failed (timeout). test/minify-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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · Use loaded minimizer functions for all helper checks. · index.js:1927-1928
src/index.js:1927-1928
🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse loaded minimizer functions for all helper checks.
Module reference objects do not expose the helper methods from their loaded exports. The remaining raw-reference checks ignore supported helper behavior.
src/index.js#L1927-L1928: pass the corresponding loaded slot function todeclaredStageso path minimizers run at their declared stage.src/index.js#L1540-L1544: pass loaded slot functions todeclaredFlagsso stats register the flags that optimization writes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: ce9187e8-0571-4e10-a734-eace277bc167
⛔ Files ignored due to path filters (2)
test/__snapshots__/parallel-option.test.js.snapis excluded by!**/*.snaptest/__snapshots__/validate-options.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
.changeset/worker-implementation-path.mdREADME.mdsrc/implementation.jssrc/index.jssrc/minify.jssrc/options.jsontest/extractComments-option.test.jstest/fixtures/minify-default-export.jstest/fixtures/minify-default-property.jstest/implementation.test.jstest/parallel-option.test.jstypes/implementation.d.tstypes/index.d.tstypes/minify.d.ts
Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review.
|
My original thinking was: if the user passes an implementation, that also means the options and the rest of the payload have to be serializable. If they aren't, throwing is the expected behavior, and the user can change what they pass. What I'm weighing now is whether to fall back silently or throw, so the user can see that the options conflict. Maybe when canMinifyByPath returns false, we should throw and say why it was refused. |
The helpers are on the loaded function, so a module reference ran at the default stage and its flag never reached stats; and two paths reporting no version shared one cache entry.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #744 +/- ##
==========================================
+ Coverage 97.75% 97.79% +0.04%
==========================================
Files 4 5 +1
Lines 1557 1634 +77
Branches 588 620 +32
==========================================
+ Hits 1522 1598 +76
- Misses 35 36 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The default minimizer is named by module path, so salting chunk hashes with it made every emitted file depend on where the checkout sits. The path now reaches the cache identity alone.
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: f6c9610a-c15e-4c79-b492-e5b434552597
📒 Files selected for processing (2)
src/index.jstest/minify-option.test.js
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
| .update( | ||
| getSerializeJavascript()( | ||
| Array.isArray(this.options.minimizer.implementation) | ||
| ? this.options.minimizer.implementation.map(getModuleRef) | ||
| : getModuleRef( | ||
| /** @type {MinimizerImplementationValue<EXPECTED_ANY>} */ | ||
| (this.options.minimizer.implementation), | ||
| ), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1900,2060p' src/index.js
sed -n '2390,2605p' src/index.js
rg -n -C 6 'renderEmbeddedSource|variesOn|embeddedSourceHash|cacheSuffix|identity' src/index.jsRepository: webpack/minimizer-webpack-plugin
Length of output: 29109
Include module references in the embedded-source cache key.
renderEmbeddedSource() uses data as its cache variation input. data contains minimizer versions and options, but not module references. identity contains the module references, but it only reaches asset optimization through cacheSuffix. With filesystem caching, two module-path minimizers with matching versions and options can therefore reuse the same embedded-source cache entry. Pass an identity-based source to renderEmbeddedSource(), and keep data for embeddedSourceHash.
…checkout Its path is read against the compiler context, so two modules reporting no version are two identities while one module hashes the same wherever it was cloned. A `Map` or `Set` holding a function keeps the task on the source path, which `Object.values` never saw.
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: 113c2225-9804-495c-991e-3b4c524cf603
⛔ Files ignored due to path filters (2)
test/__snapshots__/MinimizerPlugin.test.js.snapis excluded by!**/*.snaptest/__snapshots__/test-option.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
src/implementation.jssrc/index.jstest/embedded-source.test.jstest/fixtures/embedded/css-says-one.jstest/fixtures/embedded/css-says-two.jstest/implementation.test.jstest/minify-option.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- test/implementation.test.js
- src/implementation.js
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.
A bare specifier and a file of that name contextify alike while `require` reaches two different modules, so the identity is taken from what would be loaded.
2 participants
Summary
@xiaoxiaojx's #732, brought up to
mainand finished. Their commit is carried through as theirs; #732 can be closed in favour of this.A minimizer may now be named by module path — a string, or
{ path, export }for a named export, the waysass-loadertakes itsimplementation— and the default terser is named that way. A worker thenrequires it, instead of being handed its source and rebuilding it withnew Function, and the helpers a function carries (getTypes,filter, …) survive the crossing rather than being shipped alongside as data.The saving is per task, so it needs everything that task carries to be transferable. One minimizer written as a function anywhere in the list keeps the whole task on the source path, and so does a function anywhere in the payload — an
extractCommentscallback, or a minimizer's own options holding one — because a required minimizer is handed the payload as it is and a structured clone throws on a function. Without that second rule the default configuration plusextractComments: (astNode, comment) => …fails the build with… could not be cloned.Generators stay callable-only: nothing
requires one in a worker, and the schema takes no module path for one, so the types no longer accept what validation would reject.What kind of change does this PR introduce?
feat.
Did you add tests for your changes?
Yes. #732 brought
test/implementation.test.jsand the worker-dispatch cases intest/parallel-option.test.js; this adds the three that were missing — a mixed path-and-function list falling back totransform, a functionextractCommentsdoing the same, and a minimizer whose own options hold a function. 744 green.Does this PR introduce a breaking change?
No.
minifykeeps taking a function, and every configuration that worked keeps working; what changes is how the default reaches a worker.If relevant, what needs to be documented once your changes are merged or what have you already documented?
Documented here: the module-path form in the
minifysection, what forfeits it, and a changeset.Use of AI
Claude Code was used to bring the branch to
main, resolve the conflicts and fix what review found. The clone failure was confirmed by running the default configuration with a functionextractCommentsunderparallel: truebefore and after.Generated by Claude Code
Summary by CodeRabbit
New Features
{ path, export }references, including named and default exports.Documentation
Type Support