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

Replaced jekyll-minifier that uses uglifier by terser #2571

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

george-gca
Copy link
Collaborator

Hopefully fixes #2548.

@george-gca
Copy link
Collaborator Author

@CheariX can you test if this change fixes your issue with ES6?

Signed-off-by: George Araujo <[email protected]>
@JoJoDeveloping
Copy link

JoJoDeveloping commented Jul 13, 2024

Note that jekyll-minifier also minized HTML, CSS and JSON. Your solution no longer does this.

Actually, PRing jekyll-minifer to replace it's use of uglifier by terser seems quite straightforward -- jekyll-minifier is rather simple, your terser.rb already rewrites about half of it.

@george-gca
Copy link
Collaborator Author

@JoJoDeveloping you are right, so we can't get rid of jekyll-minifier just yet.

I made a PR for jekyll-terser with my code, but you are right. I wll send a PR for jekyll-minifier.

@george-gca george-gca marked this pull request as draft July 13, 2024 23:46
@george-gca george-gca marked this pull request as ready for review July 14, 2024 00:52
@george-gca
Copy link
Collaborator Author

I made a PR for both jekyll-minifier and jekyll-terser. If the second one is accepted, we can drop the plugin I created and use jekyll-terser instead. If the first one is accepted, we can drop both my plugin and jekyll-terser and use only jekyll-minifier.

@CheariX
Copy link
Contributor

CheariX commented Jul 14, 2024

Thank you @george-gca for taking care of this issue!

When I wrote it initially, I was not aware that al-folio does not use uglifier directly but via jekyll-minifier. Therefore, changing from uglifier to terser is more complex than expected.

I hope that your upstream PRs get merged soon.

Anyways, I tested this PR and it worked flawlessly on my site. I vote for merging this PR into main.

I also checked some of the minified JavaScript code. Looks pretty good (as far as minimized code can look "good")

Using this PR, I can finally deploy my website again 😀

@george-gca
Copy link
Collaborator Author

I was about to ask you this, so the minified js is looking right. Is this also true for the css and html files? And what about when jekyll env is production?

@CheariX
Copy link
Contributor

CheariX commented Jul 15, 2024

  • CSS looks minimized to me too. However, I'm not sure how minified CSS looks like; the CSS that we had before terser also looked minified to me, even in non-production mode.
  • HTML looks different to the minimized code that we had before terser. Before, we got only a single line. Now, it seems that JavaScript inside <script> tags is not minimized. The HTML itself, however, looks like it was minimized. I guess the reason is that terser does not touch HTML in any case. And jekyll-minifier does no longer compress JavaScript due to compress_javascript: false (which we need to get ES6 compatibility).
    • I quickly checked where we currently use embedded JavaScript. There are only a very few places (disqus.liquid, mathjax.liquid, misc.liquid, progressBar.liquid, pseudecode.liquid). I'm not sure why these files use embedded JavaScript instead of using *.js files but they could be easily refactored if this is desired. Actually, best practices is also to avoid embedded JavaScript (e.g., deployment of Content-Security Policy is easier, but we don't use it anyway at the moment)
  • Currently, JavaScript files are also minimized in non-production environments.
    • I added a code suggestion to "fix" this.
    • HTML/CSS is not touched by terser. Neither in production environment nor in non-production environment. Terser only touches JavaScript files.

@george-gca
Copy link
Collaborator Author

I thought about adding minification only on production environments, but decided not to since using it on develop environments helped me get it right. Maybe it is a good idea to add a new param somehow to let the user specify if it should run only on production or not, and by default do this.

I quickly checked where we currently use embedded JavaScript. There are only a very few places (disqus.liquid, mathjax.liquid, misc.liquid, progressBar.liquid, pseudecode.liquid). I'm not sure why these files use embedded JavaScript instead of using *.js files but they could be easily refactored if this is desired. Actually, best practices is also to avoid embedded JavaScript (e.g., deployment of Content-Security Policy is easier, but we don't use it anyway at the moment)

Thanks for the lookup. I think I will handle these guys next in this PR. I will only push it forward when the minification is as close as possible to the one with uglifier.

@JoJoDeveloping
Copy link

using it on develop environments helped me get it right.

For me, minifying takes quite a long time, on the order of several tens of seconds in addition to the rest. It makes the build much slower, and therefore I'm happy I don't have to deal with it when developing/prototyping. So please leave the option to have it production-only (or invest the effort into making it real fast 🙃).

@george-gca
Copy link
Collaborator Author

Another possibility is to migrate the solution to npm-based like already done with PurgeCSS. In this case I would use html-minifier-terser. What are your considerations about this approach?

@CheariX
Copy link
Contributor

CheariX commented Jul 16, 2024

using it on develop environments helped me get it right.

For me, minifying takes quite a long time, on the order of several tens of seconds in addition to the rest. It makes the build much slower, and therefore I'm happy I don't have to deal with it when developing/prototyping. So please leave the option to have it production-only (or invest the effort into making it real fast 🙃).

I'd second this. Minifying takes some time, so I would like to skip this step in development. Also, I find the non-minified code quite nice for feature development.

Another possibility is to migrate the solution to npm-based like already done with PurgeCSS. In this case I would use html-minifier-terser. What are your considerations about this approach?

I also thought about alternatives to jekyll-minifier. html-minifier-terser seems to be the logical solution for this. However, I have no strong opinion about that. At least, it would compress embedded JavaScript (which I think we don't need; we can remove it).

@JoJoDeveloping
Copy link

Perhaps we should do performance testing between the various alternatives?

@george-gca
Copy link
Collaborator Author

Tbh I am not really concerned about performance here, since this will only run on production environment. I am worried about maintainability, both of our code and the ones we are depending on.

For instance, jekyll-minifier last code update was 4 years ago, and it uses under the hood 4 packages: uglifier for js (last updated 4 years ago, though UglifyJS is heavily maintained), CSSminify (last updated 4 years ago), Htmlcompressor (last updated 7 years ago), and JSON::Minify (last updated 8 years ago). I am not saying that all these should be continuously updated (since JSON, for instance, is a well-defined standard), but looking at code that haven't seem an update in a while gives a certain insecurity.

Meanwhile html-minifier-terser was updated last year. Maybe it is a good moment to migrate to other solution for code minification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uglifier: Support ES6 Syntax
3 participants