Skip to content
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 injected inline JavaScript code #2115

Closed
wants to merge 1 commit into from

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Dec 10, 2019

Motivation

Perhaps this is what is called preliminary optimization, but it seems to me that this certainly will not hurt and does not bear any negative consequences, right?

For example, before and after:

<script type="text/javascript">(function() {
  function setDataThemeAttribute(theme) {
    document.querySelector('html').setAttribute('data-theme', theme);
  }
  
  var preferDarkQuery = '(prefers-color-scheme: dark)';
  var mql = window.matchMedia(preferDarkQuery);
  var supportsColorSchemeQuery = mql.media === preferDarkQuery;
  var localStorageTheme = null;
  try {
    localStorageTheme = localStorage.getItem('theme');
  } catch (err) {}
  var localStorageExists = localStorageTheme !== null;

  if (localStorageExists) {
    setDataThemeAttribute(localStorageTheme);
  } else if (supportsColorSchemeQuery && mql.matches) {
    setDataThemeAttribute('dark');
  }
})();</script>
<script type="text/javascript">(function(){function a(a){document.querySelector("html").setAttribute("data-theme",a)}var b=window.matchMedia("(prefers-color-scheme: dark)"),c=b.media==="(prefers-color-scheme: dark)",d=null;try{d=localStorage.getItem("theme")}catch(a){}var e=null!==d;e?a(d):c&&b.matches&&a("dark")})();</script>

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

See HTML markup.

@lex111 lex111 requested a review from endiliey December 10, 2019 16:24
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Dec 10, 2019
@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-preview ready!

Built with commit be24b4c

https://deploy-preview-2115--docusaurus-preview.netlify.com

@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-2 ready!

Built with commit be24b4c

https://deploy-preview-2115--docusaurus-2.netlify.com

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this improvement, but I'm not sure if we we're missing any cases where this is not wanted. Should we add an option for it? I'm leaning towards no but just wondering. Feel free to self-merge.

Copy link
Contributor

@endiliey endiliey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tbh i dislike the fact having to automatically babel transform this kind of code. Its extra dependency (and slow) and babel preset minify has a lot of problems (sometimes breaking code)

I have encountered it before in which it broke my code, and even quick google showed that sometimes it break for of loop https://github.com/babel/babel/issues/10560. I have been following babel for a while and they seem to neglect babel minify too due to manpower constraint

I think we shouldnt do this. What we can do is just minifying the dark mode fouc script fix manually and use that instead.

@endiliey
Copy link
Contributor

image

Above is the thread. Minify is kinda abandoned.

On another note, html will also be compressed by default (on netlify for example), so the actual gain might not be that much. HTML gzip compression is also very efficient unlike JS. Can we compare this netlify preview HTML gzipped size vs before this PR ?

Thoughts ?

@yangshun
Copy link
Contributor

My two cents - Passing the code through Babel is not just about minifying, there's also conversion of syntax as well. In v1 we had a few instances where our scripts were written in ES6 syntax, resulting in the page being broken for older browsers.

@endiliey
Copy link
Contributor

endiliey commented Dec 11, 2019

Hmm i guess you are right at that point, but isnt the point of this PR is more on the minifying ? I am not against the minifying. My concern is more on the fact that babel minify seems to be unmaintained and can broke someone else code. Like I’d prefer terser if we want to minify and that seems to be the standard nowadays.

Terser>UglifyES>Babel minify

On another note, do we want all this magic in core ?

This is kinda similar to how we optimize image in v1. Sometimes optipng or imagemin seems to break someone’s else image (or corrupt it).

What if user dont want to minify for sake of debuggability in production. Minification isnt perfect as well, some things might break.

Maybe its better to just minify it in the plugin itself. In this case the theme-classic plugin. This keeps core lean and simple.

Its not the case in v1 because we try to handle everything, but in v2 we can try to shift more responsibility to user and plugin.

@endiliey
Copy link
Contributor

endiliey commented Dec 11, 2019

Another example that I can think of

What if user purposefully use es modules in the injected html ? we dont want to transform that right. We need a check as well.

Its called differential loading

<script type="module">let foo = “test”;</script>
<script nomodule>var foo = “test”<script>

Edit:

my key point is more about “simplicity” in core.

The moment we put this 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)

@lex111
Copy link
Contributor Author

lex111 commented Dec 11, 2019

That's right, I did not take into account all these things, I wanted to minify inline scripts, but there are a lot of pitfalls here, which is a pity! :(
What about minifying HTML? Maybe it can be done completely during the build (regardless of the use of static hosting provider)?

image

@lex111 lex111 closed this Dec 11, 2019
@lex111 lex111 deleted the minify-inline-js branch December 11, 2019 06:05
@endiliey
Copy link
Contributor

endiliey commented Dec 11, 2019

I think we can actually minify gzip html with compression webpack plugin.

https://github.com/webpack/compression-webpack-plugin

Our html renderer (static site generator webpack plugin) is compatible with it.
See https://github.com/endiliey/static-site-generator-webpack-plugin/blob/cd502af2291b950d67fb247175046961b3356c1f/test/success-cases/compression/webpack.config.js#L23

@endiliey
Copy link
Contributor

endiliey commented Dec 11, 2019

On another thought, there seems to be two things

  1. minify the html
  2. gzip the html

Almost forgot minify and gzip is different.
But of course this all has a cost, such as build time and more memory used. So maybe its better done as a plugin. Actually if user is adventurous enough, they can always use the postBuild hook to do their own minifying thing. Or using configureWebpack to do it during webpack compilation step

@lex111
Copy link
Contributor Author

lex111 commented Dec 11, 2019

I do not think this is so critical, because the website is build via CI, that is, not on the user's local machine (in most cases).
I just want Docusaurus out of the box to be or present means for better performance. Ideally, I think we need to provide for the ability to compress images during build website...

@endiliey
Copy link
Contributor

endiliey commented Dec 11, 2019

. For html we can do it with https://github.com/DanielRuf/html-minifier-terser (its used by htmlwebpackplugin).

We can modify

and return minified html string instead

As for images, its considered best practice that if they want to optimize it, they should do it through webpack loader (so its on demand optimization) or do it on their own. See docusaurus plugin ideal image for example.

We should not touch anything in /static folder. Its been quite proven from maintaining v1 for a year+ that if we try to optimize all image, many times it broke someone else image and usually will lower the image quality, so thats why I dont think it should be in core. Optimizing image is all about lowering quality anyway.

Another thing is that its not a matter whether its build in CI/not, The thing is image processing consumes lot of memory, and time. Most people are on free plan CI and it has timeout, if they have 500 images and we optimize it all, it can most likely cause NODEJS heap out of memory or timeout. Better let them do it on demand

@lex111
Copy link
Contributor Author

lex111 commented Dec 11, 2019

Fine, although I would prefer to minify HTML as a plugin, which is enabled by default, and mention in docs about image optimization (something like recipe).

@endiliey
Copy link
Contributor

endiliey commented Dec 11, 2019

I think its fine to minify HTML in core, since minifying text is so much much faster (negligible) than optimizing images. Also note that minifying !== syntax transform (what babel/env does).

Edit: you can try https://github.com/facebook/docusaurus/tree/endi/minifyhtml for poc

wdyt ?

For image optimization, PR always welcome for docs 😉 Though I dont know where to put it also. @wgao19 is the boss for docs.

@lex111
Copy link
Contributor Author

lex111 commented Dec 11, 2019

I think its fine to minify HTML in core, since minifying text is so much much faster (negligible) than optimizing images. Also note that minifying !== syntax transform (what babel/env does).

Will try implement this. Oh you already did, cool! I will look at the result.

@lex111
Copy link
Contributor Author

lex111 commented Dec 11, 2019

@endiliey this works fine! Also, besides this, we have a minified inline JS code! 👏

@endiliey endiliey mentioned this pull request Dec 11, 2019
@wgao19
Copy link
Contributor

wgao19 commented Dec 12, 2019

#1640 recipe for image optimization

@lex111 lex111 restored the minify-inline-js branch April 18, 2020 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants