Add MinifyHtml transformer #588
Conversation
sebastianbenz
commented
Jan 28, 2020
sebastianbenz
commented
- collapses whitespace (except in
- trims whitespace (except in
- removes comments except if they match a given pattern
- minifies JSON
- minifies inline amp-script using terser
* collapses whitespace (except in <pre>, <textarea>, <script> * trims whitespace (except in <pre>, <textarea>) * removes comments except if they match a given pattern * minifies JSON * minifies inline amp-script using terser
| child.data = JSON.stringify(JSON.parse(child.data), null, ''); | ||
| } catch (e) { | ||
| // invalid JSON | ||
| this.log.warn('Invalid JSON', child.data); |
There was a problem hiding this comment.
If the JSON is invalid, does it still parse in the browser? Just wondering if this somehow fail more explicitly if that's the case (or maybe not in the scope of optimizer?)
sebastianbenz
force-pushed
the
node-cleanup2
branch
from
62f7211
to
4efcccb
Compare
| * MinifyHtml - minifies files size by: | ||
| * | ||
| * - minifying inline JSON | ||
| * - minifying inline amp-script using https://www.npmjs.com/package/terser |
There was a problem hiding this comment.
Is the <style amp-custom> tag already assumed to be minified?
There was a problem hiding this comment.
This currently happens in the SeparateKeyframes as we're already parsing the CSS there.
| * - minifying inline amp-script using https://www.npmjs.com/package/terser | ||
| * - collapsing whitespace outside of pre, script, style and area. | ||
| * - removing comments | ||
| * |
There was a problem hiding this comment.
Here's a list of additional transformations you could consider:
- removing quotes where they are not needed (
lang="en_US"=>lang=en_US) - removing empty attribute values (
data-bool-flag=""=>data-bool-flag) - removing unneeded attribute values (
disabled="disabled"=>disabled) - removing redundant attribute values where the values is the default (
<input type=number step=1>=><input type=number>) - removing optional tags: https://html.spec.whatwg.org/multipage/syntax.html#optional-tags
- decoding HTML entities into Unicode characters (
©=>©)
Probably not needed within an Amp context:
- removing
type="text/javascript"from<script>tags - removing
type="text/css"from<style>tags
There was a problem hiding this comment.
Thanks for the suggestions! I've filed #594
Some things are already handled by the parser (e.g. removing empty attribute values)
| } | ||
|
|
||
| canCollapseWhitespace(tagName) { | ||
| return !/^(?:script|style|pre|textarea)$/.test(tagName); |
There was a problem hiding this comment.
Not sure how important performance is here, but as this runs over every loop, it might be worthwhile to test using direct string comparisons here instead of a regex.
There was a problem hiding this comment.
just tested this, non-regex is 25% faster. Addressed in 62c343e
| } | ||
|
|
||
| canTrimWhitespace(tagName) { | ||
| return !/^(?:pre|textarea)$/.test(tagName); |
There was a problem hiding this comment.
Not sure how important performance is here, but as this runs over every loop, it might be worthwhile to test using direct string comparisons here instead of a regex.
There was a problem hiding this comment.
addressed in 62c343e
|
|
||
| minifyScriptNode(node, opts) { | ||
| const isJson = this.isJson(node); | ||
| const isAmpScript = this.isInlineAmpScript(node); |
There was a problem hiding this comment.
Shouldn't one of these exclude the other? No need to process the second if the first is already true.
There was a problem hiding this comment.
addressed in 62c343e
