-
-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(v2): minify html #2116
feat(v2): minify html #2116
Conversation
Deploy preview for docusaurus-2 ready! Built with commit 036fd90 |
Deploy preview for docusaurus-preview ready! Built with commit 036fd90 |
|
||
// minify html with https://github.com/DanielRuf/html-minifier-terser | ||
return minify(renderedHtml, { | ||
collapseWhitespace: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about adding these two options?
- removeAttributeQuotes
- removeEmptyAttributes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think removeAttributeQuotes can be quite unsafe according to http://jkorpela.fi/qattr.html and HTML specification recommends to always use quotes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand correctly, not all quotes are removed, but only in places where this is possible? But OK, let's do without this option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woohoo! 🎉
Motivation
This is sort of continuation from #2115, but with several differences. Thank you @lex111
1. We minify the HTML instead of only the inlined JavaScript
Smaller size of HTML
2. The minifier is based on Terser, not babel-preset-minify
As mentioned, Terser is the standard now and babel-preset-minify is no longer maintained and seems to have lot of problems (breaking code)
3. No babel syntax transform, just pure minifying
See this comment #2115 (comment) and also consider this scenario: What if user purposefully use es modules in the injected html ? we dont want to transform that with babel. Example:
The moment we babel transform inlined (injected HTML) script in core and mention “Hey, you can use ES6 syntax+ for the inlined script, we will transform and minify it for you”. People will start abusing it. And we will start seeing GitHub issue/ questions like “Inline script does not transform optional chaining”, “how do we add new babel plugin for the inlined script transform ?”, “why the inlined script cant work with dynamic import”. and so on.
But if we shift this responsibility to plugin itself and only allow the very low level API of injecting html tags in core, we dont have this problem. (minimal API)
TL;DR we should only minify (which doesnt change syntax and result in shorter string length)
3. No extra dependencies added
Since html-webpack-plugin already depends on html-minifier-terser, technically there is no new dependency added.
Have you read the Contributing Guidelines on pull requests?
yes
Test Plan
Netlify should work
Before
Results: ~4kb (parsed size) saved for /docs/2.0.0-alpha.36/advanced-plugins/index.,html
JS is minified too
![image](https://user-images.githubusercontent.com/17883920/70620094-edbc8700-1c48-11ea-9344-317f5a07db5d.png)