refactor(html): minify inline CSS and JSON through renderEmbeddedSource alone - #14
Conversation
…ce alone Source PR: webpack#21993 Source head: d851ccf
|
![shipwright-agent[bot]](https://avatars.githubusercontent.com/u/140162738?s=60&v=4){kind=small}
|
|
||
| /** | ||
| * Strip the whitespace between a JSON body's tokens, every literal copied byte | ||
| * for byte. Re-serializing would round numbers, drop a duplicate key and rewrite |
There was a problem hiding this comment.
Shipwright · HIGH
The 'stripJsonWhitespace' function silently returns the input unchanged when 'JSON.parse' throws, conflating 'not JSON' with 'malformed JSON'.
Impact: The 'stripJsonWhitespace' function silently returns the input unchanged when 'JSON.parse' throws, conflating 'not JSON' with 'malformed JSON'. A caller cannot distinguish a template/placeholder from a genuinely broken JSON body, and the comment says 'not ours to touch' for both. This makes debugging and downstream validation harder, and the behavior is not surfaced in the return type.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| _htmlSyntax || (_htmlSyntax = require("./syntax")); | ||
| const sheet = { mode: /** @type {"minify"} */ ("minify"), ...options }; | ||
| const block = { ...sheet, as: BLOCK_CONTENTS }; | ||
| // A `style=""` repeats across a document far more often than it varies. |
There was a problem hiding this comment.
Shipwright · HIGH
The builtin renderer's block memoization is keyed only by source string, but the renderer closes over mutable 'options' and the 'sheet'/'block' objects are created once per 'builti
Impact: The builtin renderer's block memoization is keyed only by source string, but the renderer closes over mutable 'options' and the 'sheet'/'block' objects are created once per 'builtinEmbeddedRenderer()' call. If a caller mutates the options object after creating the renderer, or reuses one renderer across prints with different CSS environments, the memoized 'blocks' Map can return stale minified output from a previous…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| if (isBlock) { | ||
| const memoized = blocks.get(source); | ||
| if (memoized !== undefined) return memoized; | ||
| } |
There was a problem hiding this comment.
Shipwright · HIGH
The new 'builtinEmbeddedRenderer' catches all errors from 'SourceProcessor().process()' and returns 'undefined', which the printer treats as 'leave as written'.
Impact: The new 'builtinEmbeddedRenderer' catches all errors from 'SourceProcessor().process()' and returns 'undefined', which the printer treats as 'leave as written'. This swallows not only parse failures but also programming errors (e.g., a bug in the CSS minifier or an invalid options object), making it impossible to detect regressions in production output. The old code had the same catch pattern, but the refactor expan…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
|
|
||
| /** | ||
| * Strip the whitespace between a JSON body's tokens, every literal copied byte | ||
| * for byte. Re-serializing would round numbers, drop a duplicate key and rewrite |
There was a problem hiding this comment.
Shipwright · HIGH
'stripJsonWhitespace' parses and re-emits JSON without validating the result against the original semantics beyond whitespace removal.
Impact: 'stripJsonWhitespace' parses and re-emits JSON without validating the result against the original semantics beyond whitespace removal. The hand-rolled scanner tracks only string/escape state and does not account for JSON numbers, literals, or structural tokens; a body that parses as JSON but contains unusual whitespace inside a string is copied byte-for-byte, but any future change to this scanner could silently alte…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
1 participant
Summary
The HTML printer ran its own CSS and JSON minifiers for an inline
<style>, astyle=""and a JSON<script>, so a caller'srenderEmbeddedSourcecould not own those bodies — a custom minimizer had no way to minify them itself. Now the printer minifies nothing: every nested body is offered to the renderer and a declined one is left as written.builtinEmbeddedRendererexports webpack's own CSS and JSON minifiers as such a renderer, andhtmlMinifycomposes it behind the caller's, so a custom minimizer answers for the languages it claims and webpack's own answers for the rest.What kind of change does this PR introduce?
refactor
Did you add tests for your changes?
Yes —
test/HtmlSyntax.unittest.jscovers the renderer-only path and webpack's own renderer passed behind a declining one;test/helpers/SampleEmbeddedMinifyPlugin.jscomposes it the wayhtmlMinifydoes, so thehtmlandcssconfigCasesexercise it in a real build.Does this PR introduce a breaking change?
No.
htmlMinifyandoptimization.minimize.htmlproduce the same output; the printer's CSS options (environment,convertLengthUnits,rewriteCustomProperties,cssTransforms,cssUnusedSymbols,cssPseudoClasses) move tobuiltinEmbeddedRenderer, which a directhtml.syntaxcaller passes asrenderEmbeddedSource.If relevant, what needs to be documented once your changes are merged or what have you already documented?
html.syntax.builtinEmbeddedRendererandhtml.syntax.stripJsonWhitespaceare new exports worth a line whererenderEmbeddedSourceis documented.Use of AI
Claude Code was used to write the change and its tests under my direction and review, and to run the unit suites, the
htmlandcssconfigCasesin both the plain and filesystem-cache runs, and the lint chain.Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Source merge-base:
ee66ae80b8726db4e2b5e247a6ff89c7fbb3cab9Source head:
d851ccfc9a17a33a0cc139760ca6efc7791e0770