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

Make minify rename javascript variables to make javascript even shorter #7311

Closed
kssilveira opened this issue May 24, 2020 · 18 comments
Closed

Comments

@kssilveira
Copy link

If I minify javascript using yui-compressor:

$ echo "function init() {
  var nodes = document.getElementsByTagName('iframe');
  for (var i = 0; i < nodes.length; i++) {
    var data = nodes[i].getAttribute('data-src');
    if (data) {
      nodes[i].setAttribute('src', data);
    }
  };
}
setTimeout(init, 1500);
" | yui-compressor --type js

I get

function init(){var a=document.getElementsByTagName("iframe");for(var b=0;b<a.length;b++){var c=a[b].getAttribute("data-src");if(c){a[b].setAttribute("src",c)}}}setTimeout(init,1500);

If I use hugo --minify, I get:

function init(){var nodes=document.getElementsByTagName('iframe');for(var i=0;i<nodes.length;i++){var data=nodes[i].getAttribute('data-src');if(data){nodes[i].setAttribute('src',data);}};}
setTimeout(init,1500);

This request is to make hugo --minify rename javascript variables to make javascript even shorter.

What version of Hugo are you using (hugo version)?

$ hugo version
Hugo Static Site Generator v0.70.0/extended linux/amd64 BuildDate: 2020-05-13T17:30:34Z

Does this issue reproduce with the latest release?

I think I have the latest release.

@stale
Copy link

stale bot commented Oct 4, 2020

This issue has been automatically marked as stale because it has not had recent activity. The resources of the Hugo team are limited, and so we are asking for your help.
If this is a bug and you can still reproduce this error on the master branch, please reply with all of the information you have about it in order to keep the issue open.
If this is a feature request, and you feel that it is still relevant and valuable, please tell us why.
This issue will automatically be closed in the near future if no further activity occurs. Thank you for all your contributions.

@stale stale bot added the Stale label Oct 4, 2020
@kssilveira
Copy link
Author

I think this is still relevant and valuable because it can make pages load faster.

@stale stale bot removed the Stale label Oct 4, 2020
@tdewolff
Copy link

tdewolff commented Oct 8, 2020

This has been implemented in https://github.com/tdewolff/minify/releases/tag/v2.9.0 and you should be able to update the minify package manually. Currently Hugo doesn't use the latest version of minify that has this feature.

@jeremielp
Copy link

Is there a plan to integrate this update into hugo? Or shall we go with manual update still?

@moorereason
Copy link
Contributor

@bep, I think we can evaluate updating minify again.

To recap the situation:

We updated from v2.6.2 to v2.9.4 in #7701, but that was reverted in #7792 after this report from the forums. I was able to reproduce the issue at the time but couldn't identify the root cause.

Today, I took another look at the issue reported in the forum, and the problem is fixed in v2.9.9 (specifically, tdewolff/minify@eb4ff2b).

Cc: @tdewolff @hrishikesh-k

@bep
Copy link
Member

bep commented Dec 1, 2020

I guess this issue is talking about the --minify flag, but if you're talking about external JS files, note that js.Build also has an minify option, which I'm pretty sure is doing a great job:

https://gohugo.io/hugo-pipes/js#import-js-code-from-assets

As to updating the upstream minifier. We have had a couple of breaking changes with this recently, and the problem with these breaks is that they don't just put a scratch in your site. We currently don't have a good enough test suite on the Hugo side of the fence to make sure that all is OK, so upgrades requires manual work, and this has not been a priority of me lately.

@jeremielp
Copy link

jeremielp commented Dec 1, 2020

I though about that too. I have been checking the code proposed by @kssilveira with these 2 methods:

  • hugo --minify applied to inline JS in baseof
  • asset minification using Hugo Pipes
    {{ $js := resources.Get "test.js" }}
    {{ $jsm := $js | resources.Minify }}

The output is the same as the one mentioned in the issue in both cases.

@bep
Copy link
Member

bep commented Dec 1, 2020

The output is the same as the one mentioned in the issue in both cases.

No, I meant something like this:

{{ $js := resources.Get "test.js" }}
{{ $jsm := $js | js.Build (dict "minify" true) }}

@jeremielp
Copy link

Yes, that one renames variables correctly

@kssilveira
Copy link
Author

kssilveira commented Jan 13, 2021

Thanks, js.Build worked indeed for resources. I ran into issues caused by the renaming of global variables (that I wanted to access from other files or that clashed with global variables in other files), this could be one of the reasons for "As to updating the upstream minifier. We have had a couple of breaking changes with this recently".

Another use case I have for this is to call js.Build on the result of a partial that generates JS code that is different for each page. Do you have a suggestion for how to do that?

I ran into a few problems:

  • js.Build expects a resource, so I had to use resources.FromString
  • resources.FromString would cache the resource across pages, so I had to pick a different name for the resources corresponding to each page
  • I could not prevent the partial from escaping the JS code (e.g. < becomes &lt;)

I was experimenting with:

{{- $js := partial "main.js" . | safeJS | resources.FromString (printf "%s%s" .Name "partial.js") -}}
{{ $js.Permalink }}
<script type='text/javascript'>
  {{ $js.Content | safeJS }}
</script>

And the idea was to do:

{{- $js := partial "main.js" . | safeJS | resources.FromString (printf "%s%s" .Name "partial.js") | js.Build (dict "minify" true) -}}
<script type='text/javascript'>
  {{ $js.Content | safeJS }}
</script>

@kssilveira
Copy link
Author

This worked (after creating the file inside assets/main.js):

{{- $js := resources.Get "main.js" | resources.ExecuteAsTemplate (printf "%s%s" .Name "partial.js") . | js.Build (dict "minify" true) -}}
<script type='text/javascript'>
  {{ $js.Content | safeJS }}
</script>

@jeremielp
Copy link

Another use case I have for this is to call js.Build on the result of a partial that generates JS code that is different for each page. Do you have a suggestion for how to do that?

Also interested in this. That would be a good workaround to the upstream minifier issue, at least for my setup.

@tdewolff
Copy link

The problems that @bep mentions are ones from a long time ago and have been fixed since. Using the updated github.com/tdewolff/minify package you'll get variable renaming as well (and usually a little faster and better than esbuild too). Both are very high-quality libraries by the way, but they serve slightly different purposes.

The lack of tests in Hugo for the minifier was what was holding back the upgrade, however is there such a test set for esbuild? If anything, minify has a very complete and extensive test set (bigger than esbuild and most other parsers). They have had a number of severe bugs as well (any bug will completely break a website by the way, it's hard to make just a scratch), like minify, which is completely normal in the development of software. I'm really unsure what the objective reasons are for not upgrading minify, besides from the "it broke stuff in the past" rhetoric.

@regisphilibert
Copy link
Member

regisphilibert commented Nov 26, 2021

The Hugo minifier (for html files) should not rename variables by default as you might be using them outside of the internal script (Alpine.js is a use case where you needed to and still can add your data script as internal while needing its declared variables outside of it.

@jmooring
Copy link
Member

jmooring commented Nov 26, 2021

1) We are now using tdewolff/minify 2.9.22 (latest).

2) The default configuration is keepVarNames = false

3) As JS, this:

function init() {
  var nodes = document.getElementsByTagName('iframe');
  for (var i = 0; i < nodes.length; i++) {
    var data = nodes[i].getAttribute('data-src');
    if (data) {
      nodes[i].setAttribute('src', data);
    }
  };
}
setTimeout(init, 1500);

is minified to this:

function init(){for(var b=document.getElementsByTagName('iframe'),a=0,c;a<b.length;a++)c=b[a].getAttribute('data-src'),c&&b[a].setAttribute('src',c)}setTimeout(init,1500)

4) As an HTML file, this:

<script>
function init() {
  var nodes = document.getElementsByTagName('iframe');
  for (var i = 0; i < nodes.length; i++) {
    var data = nodes[i].getAttribute('data-src');
    if (data) {
      nodes[i].setAttribute('src', data);
    }
  };
}
setTimeout(init, 1500);
</script>

is minified to this:

<script>function init(){for(var b=document.getElementsByTagName('iframe'),a=0,c;a<b.length;a++)c=b[a].getAttribute('data-src'),c&&b[a].setAttribute('src',c)}setTimeout(init,1500)</script>

Unless I'm missing something, this was resolved a while ago.

@tdewolff
Copy link

The minifier does not rename variables that could be referenced outside the script, i.e. all global variable names are kept even as KeepVarNames == False. If this is causing a problem, could you open an issue at https://github.com/tdewolff/minify please?

As for @jmooring the JS example looks correct, but the HTML example not, is the JS minifier enabled in that case? It should be!

@jmooring
Copy link
Member

@tdewolff Thank you. You are correct. I have updated my comment.

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants