feat: minify HTML on build #2537
Conversation
|
IMO html-minifier definitely needs custom options and most importantly this shouldn't happen in dev mode. This can change the rendering with the default options. The safest approach would be to use conservative collapse. My suggestion is to use this config: {
collapseBooleanAttributes: true,
collapseWhitespace: true,
conservativeCollapse: true,
decodeEntities: true,
minifyCSS: {
level: {
1: {
specialComments: 0
}
}
},
minifyJS: true,
minifyURLs: false,
processConditionalComments: true,
removeAttributeQuotes: true,
removeComments: true,
removeOptionalAttributes: true,
removeOptionalTags: true,
removeRedundantAttributes: true,
removeScriptTypeAttributes: true,
removeStyleLinkTypeAttributes: true,
removeTagWhitespace: false,
sortAttributes: true,
sortClassName: true
} |
|
Also, it goes without saying, we need to test this a lot before landing. :) About the env, I made #2540 which you can use. |
|
To maintainers: I just tried this PR, and I was right; it does changes the rendered HTML. It shouldn't land with the default options. Also, we definitely need this to run only for production because it's a lot slower. But I think we should also address #2524 and #2523. html-minifier is using uglify-js which doesn't work with ES6. Plus we need to be extra careful with semicolons missing in the inline JS. So, I'd put this on hold for now. Patch for reference master...XhmikosR:feat--minify-HTML-on-build |
|
Given the benefits of compression during HTTP transfers, is the cost/work of safely implementing HTML minification worth it? The current main page HTML is 4.2 Kb over the wire after compression. Even if HTML minification saved us a whopping 15%, that would be around 600 bytes. Is that worthwhile when it risks messing up rendering and complicating testing/debugging, even if those risks are small? |
|
I thought about it more, too. In total, it reduces the total HTML a lot. It's ~11K files. That being said, the gain from this after compression won't be big. At least for the English pages. For the translations, where people keep commented out the English strings too, it helps a lot more. Perhaps my config is too strict hence why I get such times. I haven't tried a production deploy without uglify for example. This should speed things up, maybe to an acceptable number. That being said, since we use Cloudflare, we could also just enable HTML minification there alternatively. I'd say, let's keep this open for a little more. I'd like to experiment more with this later after we clean up some of the other PRs. |
|
BTW with my config, the output doesn't change. This is a known issue due to inline-block elements. |
|
@Trott I think this should be closed since I can't push my changes to @nschonni's fork. I have updated the patch on my fork master...XhmikosR:feat--minify-HTML-on-build and the plan is to try it again after #2543 is merged so that we don't minify inline JS which seems to cause the biggest slowdown. |
@nschonni Does that sound good to you? |
|
Yeah, there is not much in this PR besides adding it to the pipeling. |
XhmikosR
force-pushed
the
feat--minify-HTML-on-build
branch
from
644abef
to
9b26e54
Compare
|
I rebased this and pushed it to @nschonni's branch. |
XhmikosR
force-pushed
the
feat--minify-HTML-on-build
branch
2 times, most recently
from
7558c8c
to
701ef8a
Compare
|
I need to investigate this a little more. It would certainly help if we had PR previews. |
XhmikosR
force-pushed
the
feat--minify-HTML-on-build
branch
from
76698be
to
5d052a4
Compare
|
@nschonni If you still want this (and my objection is minimal because if this ever does cause a problem, it would be easy to disable), can you resolve the conflict? |
XhmikosR
force-pushed
the
feat--minify-HTML-on-build
branch
from
0a68827
to
16fdade
Compare
|
Rebased. I'm still not sure this is worth it for nodejs.org. For translations it has bigger gains for sure. But on the other hand we don't minify anything in core docs either. That being said, personally, I always do this in my projects when possible. Also, with the changes I made, it should be "safe" layout-wise. We could always land it and see how it goes. Here's a live preview: https://hopeful-bartik-af0f39.netlify.com/en/ @nodejs/website please check if everything looks good, mostly in translations. |
|
Also, we've yet to see the slowdown when deploying to production. CI doesn't run the full build but it's almost 2.5x slower. |
|
From what I can tell based on a few tests, this reduces the page size by less than 20% (before compression, likely less after compression), while adding a dependency, making the build pipeline more complex and the output harder to read for humans. The vast majority of our traffic comes from downloads, not HTML pages. Is this tiny reduction really worth it? |
|
I don't feel super strongly about this, but the benefit of minification (even if it's small), is that not everyone that is reading the docs will have fast internet connections. |
Does this apply to the docs? I was under the impression that the website build pipeline does not touch the docs at all, since we generate the HTML files in node core. But I might be wrong, so let's assume that this does apply to the docs. The largest doc file right now is the So how much does this benefit users with a slow internet connection? Let's assume that the user is on a very old 3G connection, and in a moving vehicle. That leaves us with an estimated download rate of 348kbit/s. At this network speed, saving 8 KB equals saving 188 milliseconds. (On the first page load only, after that, the browser will cache the document and the benefit diminishes.) |
Nope, the core docs are generated in the main repo so this change doesn't apply there. I think the wise thing would be to not land this patch. |
|
Let's see if the author or one of the approving reviewers is against closing, ping @nschonni @MylesBorins @aymen94. |
|
Alright, I'll rebase tomorrow and merge it and we see how it goes. I went with the settings that from my experience find the safest. The only thing I'm not sure about is the decode entities setting, which is what I wanted people to test with translations on #2537 (comment) |
|
For what it's worth I'm not necessarily advocating for merging – just for us to make a more informed decision based on previous context we have around usage and if it'll be a meaningfully impactful change :) |
XhmikosR
force-pushed
the
feat--minify-HTML-on-build
branch
from
16fdade
to
56ab55e
Compare
|
Rebased and also updated the Preview site https://hopeful-bartik-af0f39.netlify.com/en/. Do note that the preview site is using gzip while we use brotli on prod |
XhmikosR
force-pushed
the
feat--minify-HTML-on-build
branch
from
0e22424
to
312f399
Compare
|
@MaledongGit why was this squashed? Please do not blindly merge. I kept the patches separate for a reason. I was the author of the last 2 patches. |
|
I'm gonna have to stay away from this repo until this never happens again. |
|
Also just FYI, this takes ~7 min to build on prod. Before this patchThis patchIMO the gains don't justify the slowdown. Instead we could just use Cloudflare's minify HTML for this site only. /me is out |
|
@XhmikosR if this is very important to you, we have to agree and formalise
how commits should be merged in this repo. Without it, there are no rules
that collabs can play by and these kinds of replies are intoxicating IMO.
…On Sun, 24 Nov 2019 at 08:34, XhmikosR ***@***.***> wrote:
@MaledongGit <https://github.com/MaledongGit> why was this squashed?
Please do not blindly merge. I kept the patches separate for a reason. I
was the author of the last 2 patches.
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
<#2537?email_source=notifications&email_token=AAJMWE7E2QNBASVNR2ZHO3LQVIU7PA5CNFSM4IUSFYM2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFAFQOI#issuecomment-557865017>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJMWE5R7GHJ7VLYDEZCBMTQVIU7PANCNFSM4IUSFYMQ>
.
|
|
How is it intoxicating? I've asked numerous times to not change my commits or my commit messages, yet it keeps happening. I don't have any other way to communicate this. Why would it seem normal to someone to squash patches from different authors is beyond my understanding. This is not fair to say the least and causes issues. For example, let's say this specific set of patches breaks something, but it's not @nschonni's initial patch. Why would @nschonni be blamed for my patches and vice versa? I'm not sure how to document this. It's common sense for me to not squash patches from different authors without consent or if they are not trivial. |
There are other ways to communicate this such as explicitly requesting in the PR or attempting to make policy changes. Threatening to stop contributing without offering a solution isn't super helpful. That being said it seems to me you might not know how to go about making a change to how the repo is maintained. The rules for how things should be landed can be found in our Collaborator Guide. It currently has no rules about squashing or not squashing... it specifically states how long PRs must remain open, that there must be consensus, and that it must run through CI if there are changes to executable code. If you would like to implement new rules it could be done by submitting a PR against the collaborator guide and building consensus around those changes. TBH I'm a fan of squashing PRs into a single change unless there are explicit technical reasons to keep things separate. For example the PR to land the new ESM implementation in core was opened by me, co-authored by 5 people, originally made up of 20+ commits. We squashed to one to simplify maintenance, history, and backportability.
I don't think this would ever happen TBH. We definitely do not operate any of the repos in the node.js org in a "blame" fashion. If something is broken, we fix it. I'm a huge fan of blameless post mortem's, looking at what allowed something to fail rather than blaming an individual. For example @MaledongGit did absolutely nothing wrong by landing this PR the way they did. If you think there is a problem, it is with our rules and procedures (as mentioned above). So lets work on trying to change those. Although I will be honest that you will have to convince me about why the extra effort of preserving commits it worth the extra effort, I do promise to approach it with an open mind.
Open a PR against the collab guide and lets take it from there. I'm sorry this has been so frustrating for you. |
I wasn't threatening anyone, I was actually just mentioning my plans. But I do see how this may have sounded. Sorry for that, please read below why I was frustrated by this and some background info.
I mean this in the
Well, to me, every change should be credited to person who did it. It's just the ethical thing to do, even if it's something trivial. The fact that GitHub allows us now to squash PRs does not mean it should be done blindly. My patches in this PR were seemingly pretty trivial, but I'm a huge fan of giving credits where credits are due. It's a principle I follow all my life so far. So, in order for me to provide this trivial patch, I had to do numerous tests and check if and when things break, which they did in the original patch.
I wouldn't mind if it wasn't the first time someone changed my commit message to something I don't agree, and I have expressed my discomfort numerous times (in PRs though) without any success.
I'll see what I can do. I'm not so good with words, and English isn't my native language. Also, I'd rather do something that has a biggest impact than docs, but I do acknowledge that docs are needed in big organizations, so I'll try to put something together. |
|
Forgot to comment on this:
This is what's wrong IMHO. He squashed patches from different authors (me) without their consent. |
|
Maybe the whole "We shouldn't squash by default in this repo" conversation might be better to have over in the discussion board? I think it's a perfectly fine conversation to have. The way this repo is managed has changed a lot in the last year or so and it would be good to get some documentation/agreement on what we do and how we do it. |
If you think it is wrong it is the process not the person, is what I was trying to point out
Perhaps we can enforce "if you are squashing comomits please include a co-authored-by label. AFAIK github does automatically generate this when squashing. |
|
and to what trott said lets move this discussion to either an explicit thread or the discussion board so that we can keep it documented in a single place that is focused on improving our landing guifdelines |
|
Is there a discussion board? Not super familiar with your internals, so bear with me :) |
https://github.com/orgs/nodejs/teams/website Sorry, I forgot that on GitHub, the discussions are for teams and not repos. So it would be that discussion board above. |
Regardless of squashing, there was an active, ongoing discussion about whether this should land or not. The PR was approved, but that does not always mean that it is ready to land. Personally, I am still not convinced that this is a good addition. |
|
I agree. See also my previous comment about the build time and size
differences.
We could just enable Cloudflare's HTML minification alternatively
…On Sun, Nov 24, 2019, 23:35 Tobias Nießen ***@***.***> wrote:
@MaledongGit <https://github.com/MaledongGit> did absolutely nothing
wrong by landing this PR the way they did.
Regardless of squashing, there was an active, ongoing discussion about
whether this should land or not. The PR was approved, but that does not
always mean that it is ready to land. Personally, I am still not convinced
that this is a good addition.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2537?email_source=notifications&email_token=AACVLNPRLPHWVXAE43VS5MLQVLXQXA5CNFSM4IUSFYM2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFAVPCQ#issuecomment-557930378>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACVLNMQSJ43BJGE2PDXWF3QVLXQXANCNFSM4IUSFYMQ>
.
|
This reverts commit 0cc3648. Refs: nodejs#2537
No description provided.