feat(performance): three more hints, and group the options before any ship - #11
Conversation
… ship Source PR: webpack#21961 Source head: af3b149
⛔ Shipwright · BlockedRecommendation: do not merge PR #11 · Tier
Findings (7)
Fireworks usage: 65,101 input · 720 output · 65,821 total tokens · $0.0148 · 18s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
![shipwright-agent[bot]](https://avatars.githubusercontent.com/u/140162738?s=60&v=4){kind=small}
| /** | ||
| * Returns function to determine if the connection is active. | ||
| * @param {ModuleGraph} moduleGraph module graph | ||
| * @returns {null | false | GetConditionFn} function to determine if the connection is active |
There was a problem hiding this comment.
Shipwright · CRITICAL
The new HarmonyImportBareSideEffectDependency.getCondition returns true when refModule is falsy, which keeps the connection active for unresolved modules.
Impact: The new HarmonyImportBareSideEffectDependency.getCondition returns true when refModule is falsy, which keeps the connection active for unresolved modules. This can retain broken/optional imports that previously would have been pruned, changing bundle output and potentially masking resolution errors.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| module.wasted | ||
| )} extra)` | ||
| ) | ||
| .map((module) => { |
There was a problem hiding this comment.
Shipwright · HIGH
DuplicateModulesWarning now assumes module.entrypoints is always an array with a length property.
Impact: DuplicateModulesWarning now assumes module.entrypoints is always an array with a length property. The typedef says entrypoints is string[] only when more than one, but the code accesses module.entrypoints.length unconditionally, which will throw if entrypoints is undefined for single-entrypoint modules.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| */ | ||
| constructor(modules, total) { | ||
| const list = modules | ||
| .map((it) => `\n ${it.name} (${it.loaders.join(", ")})`) |
There was a problem hiding this comment.
Shipwright · HIGH
The new warning messages interpolate module names, loader names, and statement locations directly into user-facing output without escaping.
Impact: The new warning messages interpolate module names, loader names, and statement locations directly into user-facing output without escaping. A malicious package name or loader identifier containing control characters could inject terminal escape sequences or spoof log output.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| */ | ||
| constructor(assets, total) { | ||
| const list = assets | ||
| .map((it) => `\n ${it.name} (${it.size} bytes)`) |
There was a problem hiding this comment.
Shipwright · HIGH
UnusedAssetsWarning and UnusedModulesWarning report file sizes and statement locations derived from build internals.
Impact: UnusedAssetsWarning and UnusedModulesWarning report file sizes and statement locations derived from build internals. If these values are attacker-influenced via package metadata or source content, they could be used to exfiltrate build details or craft misleading diagnostics.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
1 participant
Summary
Three checks for things nothing reported, one graph fix that made two of them possible, and a pass over the option surface while it is still free to change — none of the checks has been released, so merging and renaming costs nothing.
sourceMaps— a productiondevtoolthat writes the map into the bundle, and a loader that rewrites code without returning a map. The second leaves webpack mapping positions to the loader's output as though it were the file: a loader injecting ten lines ships a map whosesourcesContentis the loader's output and whosethrowsits on line 16 where the file has it on line 6, with no error and no warning.unusedAssets— an asset file emitted for an import whose binding nothing reads. With every hint enabled the build shippedunused.pngand said nothing.unusedModules— a module bundled although nothing uses what it exports, naming the re-export or the side-effect statement that kept it. webpack already computed the statement and buried it in an optimization bailout, which stats show only when asked and the cache does not restore.The graph fix:
experiments.futureDefaultsmarks asset modules side-effect-free, which correctly drops an import whose binding nothing reads — but also droppedimport "./favicon.png", where emitting the file is the only thing the import can have been written for. The two are now told apart at the declaration, by a subclass rather than a flag, so the far more common bound form keeps its class and no instance grows: both carry the same twenty fields, and over a 2000-module build the dependency count is unchanged at 6400.The option pass took 33 checks to 27.
unusedConfigreplacesunusedAliases,unusedDefines,unusedExternalsandunusedRules— four switches for one finding, configuration nothing used, where the fix is the same each time and each check keeps its own message.sourceMapsreplacesembeddedSourceMapsandmissingSourceMaps.unusedModulesabsorbsunusedReexports, andduplicateModulesabsorbsentrypointOverlap, which was a strict subset of it once the bug below was fixed.That bug:
duplicateModulescounted chunks per module, so a module concatenated into two entrypoints looked like it was in none and went unreported —entrypointOverlapsaw it only because it expanded concatenations. It now expands them too, and names the entrypoints paying for each copy.Refs webpack#17122
What kind of change does this PR introduce?
feat
Did you add tests for your changes?
Yes — 18 new cases under
test/configCases/performance/andtest/configCases/asset-modules/, with theentrypoint-overlap*cases migrated toduplicate-modules-*. Every guard is mutation-checked, each mutation failing a named case: dropping theuseSourceMap, has-loaders orSourceMapSourcecheck failsmissing-source-maps; ignoring a bare import, accepting any dependency class, or dropping the asset-type check failsunused-assetsandunused-assets-inline; ignoring the re-export, the bound-import or the chunk guard failsunused-modulesandunused-reexports-side-effects-free; always using the base side-effect class failsasset-modules/bare-import-side-effect-free.Two guards were removed as unreachable after mutation showed nothing depended on them, and one of those removals was wrong and is back: a module in no chunk has no runtime, which is the only place
hasOnlyUnusedExportsreads usage in, so it answered "all exports unused" for a module whose export was used.Does this PR introduce a breaking change?
No. Every check is off by default and reports nothing unless
performance.hintsis set, and the renamed options have never been in a release. The one behaviour change is on theexperiments.futureDefaultspath, where a bareimportof an asset now keeps emitting its file instead of dropping it.If relevant, what needs to be documented once your changes are merged or what have you already documented?
The
webpack.js.orgperformance page needs the 27-option surface: the three new checks, and the four merges. A companion change is drafted but not yet opened, since it documents the pre-merge names.Use of AI
Claude Code was used to survey the gap and implement this, under review at each step. Several proposals were built and then dropped because measurement contradicted them: a hint for chunks always loaded together (a branch picking one of two
import()s is indistinguishable fromPromise.allof both — both give parent set[main]), one forresolve.extensionsordering (.tsleads webpack's own default, so it would fire out of the box on any project without.tsfiles), and one for a package lying insideEffects: false(the danger did not reproduce).unusedModulesshipped narrower than first written, because the test suite showed it reporting everyimport "./polyfill"— deliberate, and a lineunusedReexportshad already drawn. Memory was measured within one tree rather than across two checkouts, after a cross-checkout comparison proved confounded: the per-module field costs ~60 bytes and is written only when the hint is on, so with it off no module carries it and retained heap is unchanged over a 2000-module build where every module has a side effect.🤖 Generated with Claude Code
https://claude.ai/code/session_01FxbjLoCKW57MoXMHEzmruX
Generated by Claude Code
Summary by CodeRabbit
"all"boolean-attribute collapsing and URL-attribute normalization in HTML minimization.performance.unusedConfig.performance.sourceMaps.performance.unusedAssetsand expandedperformance.unusedModulesreporting.Source merge-base:
ea21f562d9712d81798d551f34015524873ce7efSource head:
af3b149ebd0380866df0e40ddc9201ba61a1cc34