fix minifyJS when used outside of Node.js

[alexlamsl]

Intermittent failures in online tests is primarily due to missing character statistics reset in minifyJS().

Redone the non-Node.js version by referring to the official version, also extracted common logic for minifyCSS() and minifyURLs() so we don't have to reload module and repopulate default option values for every invocation.

[alexlamsl]

@kangax @XhmikosR so I decided to go nuclear and extract UglifyJS.minify() and build it into assets/uglify.js directly.

This way we'll be more future-proof, plus we get to simiplify minifyJS(). The only downside I can see so far is npm run assets/uglify-js looking unwieldy 😅

[XhmikosR]

I just don't get why we need that custom command to generate it. I'd rather if we kept it official.

[alexlamsl]

@XhmikosR so when uglify-js is running within Node.js, it uses UglifyJS2/tools/node.js

However, when it packages itself for the web, it does not include that file since it uses Node.js libraries.

Since UglifyJS.minify is defined within UglifyJS2/tools/node.js and for the logic path we use does not involve/require Node.js, we use it directly instead of duplicating (wrongly) the same logic inside htmlminifier.js

[alexlamsl]

The old code in minifyJS might be what UglifyJS.minify does in a prior version of uglify-js, but it certainly isn't so now and contains at least two bugs (does not reset character statistics; does not pass options.compress to Compressor) so it's actually more fragile if we go down the path of updating the duplicated code.

As for official version of uglify-js - we are still using it, just appending an extra definition of UglifyJS.minify at the end of the file, which is also extracted from the official source.

[alexlamsl]

Rebased - but I think I still need to convince @XhmikosR about appending extra code to assets/uglify.js before merging

[alexlamsl]

@XhmikosR currently, minifyJS has two different code paths calling uglify-js for Node.js and online versions. The online code path is broken, either since day one or has become out-of-sync with the upstream project's API. We have a few options to fix this:

1. fix the online code path by copying from tools/node.js
   • need to manually track upstream changes to maintain consistent behaviour
2. merge Node.js and online versions into latter
   • consistent behaviour
   • need to manually track upstream bug fixes
   • harder to discover and use for Node.js users
3. (current PR) merge both versions into require('uglify-js').minify
   • need to append a function in tools/node.js to official version of uglify.js
   • consistent behaviour
   • no DRY issues, all code are from UglifyJS2 repository

[alexlamsl]

I have now modify uglify.js generation once again by using browserify in the same way as cleancss-browser.js and relateurl-browser.js - does that work better for you @XhmikosR?

Would it make sense to call the output file uglify-browser.js?

[XhmikosR]

Sorry for the lack of replies, I was busy :/

Personally, I'd like to keep things as vanilla as possible. The problem I see with your solution is that you require a custom step even for node; this shouldn't be needed.

Anyway, whatever @kangax prefers.

[alexlamsl]

@XhmikosR no worries - we all have day job(s) 😉

Which custom step for Node.js are you referring to here? Do you mean fromString and inline_script?

[alexlamsl]

Looking through all of this - is dist/* for web consumption only? I ask because Node.js users are using src/* directly as specified by package.json.

If that's the case, why don't we get rid of assets/* altogether, and just browserify src/htmlminifier.js and put that under dist/?

[alexlamsl]

Nope, cli.js uses dist/htmlminifier.min.js for some very interesting reasons...

[kangax]

I think this LGTM so merging. Was minifyJS not working in a browser before?

[alexlamsl]

@kangax that's right - minifyJS work differently in the browser than on Node.js before, but the arrangement of test cases in the past conveniently hides the problem.

Now that I'm expanding on tests/minifier.js... 💣💥

[oliviertassinari]

That doesn't play well with webpack:

WARNING in .//html-minifier/src/htmlminifier.js
Critical dependencies:
649:15-28 the request of a dependency is an expression
@ .//html-minifier/src/htmlminifier.js 649:15-28

And later

webpack:///./~/html-minifier/src ^./.*$:13
return map[req] || (function() { throw new Error("Cannot find module '" + req + "'.") }());
^
Error: Cannot find module 'relateurl'.

[alexlamsl]

I have zero idea how webpack works, so PR welcome 😉

[oliviertassinari]

Let me have a look at it.