html-minifier-terser by juj · Pull Request #9990 · emscripten-core/emscripten

juj · 2019-12-08T13:49:29Z

html-minifier was buggy in minifying JS code, and author decided to drop support for minifying JS code in it altogether, so migrate to using html-minifier-terser that can minify <script> content inside HTML files.

PR #9989 should land first, this is easier to rebase on top of that.

…ser in PR emscripten-core#9990.

juj · 2019-12-08T14:52:46Z

Hmm, I think CI needs to be updated to run npm install before the test suite runs? Not sure where would be the best place to add that in the CI config though.

sbc100

yes, we need to to do a couple of things before landing this:

Have emsdk automatically run npm install post installation.
Teach our CI to run npm install
Inform git users / developers that we now have this post-clone step.

I wonder if we are going have other steps (such as pip install or git submodule update), in which case it might make sense to wrap them in single ./bootstrap.sh/bootstrap.bat. Maybe that is thinking too far ahead and we should see how it goes with just npm install?

package.json

juj · 2019-12-09T07:05:52Z

1. Have emsdk automatically run `npm install` post installation.

emscripten-core/emsdk#404 will resolve that.

juj · 2019-12-09T07:20:50Z

3\. Inform git users / developers that we now have this post-clone step.

Added an error message explaining to run npm install to set up dependencies.

…ser in PR emscripten-core#9990.

* Complete migration of stack overflow checks to library function in MINIMAL_RUNTIME, and add support for --separate-asm, MODULARIZE and ALLOW_MEMORY_GROWTH with MINIMAL_RUNTIME. * Use acorn-optimizer to minify <script> tags in .html code (TODO: Migrate to a better minifier, but needs one installed alongside the repository) * Remove manual JS code minification in HTML5, to use html-minifier-terser in PR #9990. * Update minimal_runtime test

juj · 2020-01-26T09:32:03Z

Brought this up to latest, and tests passing now - this is good for review and landing.

juj · 2020-01-26T17:03:48Z

There seems to be a test failures with test_cubegeom_normal_dap_far_glda_quad (test_browser.browser) in Firefox but not in Chrome that pops up, I wonder if Firefox had regressed on something. Although the failure occurs only on CI, unable to reproduce locally.

sbc100

Nice! lgtm with a couple of comments.

Should we wait a little bit, and maybe do an emscripten release that contains #9989 before we land this to make sure the any npm issues are shaken out?

emcc.py

sbc100 · 2020-01-27T22:37:43Z

Re-running failing tests on circle to see if those flakes

juj · 2020-01-28T09:51:55Z

Should we wait a little bit, and maybe do an emscripten release that contains #9989 before we land this to make sure the any npm issues are shaken out?

How important do you feel that is? I am working a bit tight now against our code freeze for 2020.01 release to try to squeeze new Emscripten compiler update in with as many features as I can.

sbc100 · 2020-01-28T20:09:03Z

Should we wait a little bit, and maybe do an emscripten release that contains #9989 before we land this to make sure the any npm issues are shaken out?

How important do you feel that is? I am working a bit tight now against our code freeze for 2020.01 release to try to squeeze new Emscripten compiler update in with as many features as I can.

Well I guess we have decided to go down this route, but we are having issue with npm on the emscripten-releases builders right now, so perhaps at least wait until the dust settles there and we can cut a new emscripten release (I hope that will be today).

juj · 2020-02-03T11:23:11Z

Iiuc the Closure related issues have all been resolved now? I wonder if this would be good to land? @kripken @sbc100 ?

sbc100 · 2020-02-03T20:18:29Z

Yes the closure related issues have now (hopefully) been solved. But I did causes a lot of issues, mostly related to window. We just cut and emscripten release containing this change so we can see how it effects actual users.

I suspect that closure will be the outlier here in terms of the fallout, since it has the native and java and js versions in there, and other dependencies should be pure JS. However, I would still rather let the dust settle a little before landing more npm dependencies. Could we at least wait a couple of days?

…rop support for minifying JS code in it altogether, so migrate to using html-minifier-terser that can minify <script> content inside HTML files.

sbc100

Sorry.. I had this comment sitting there unsent..

tools/shared.py

sbc100 · 2020-02-07T22:26:32Z

Have we seen any issues with the use of npm in the wild @kripken? Are we ready to land this change which extends the npm usage do you think?

kripken · 2020-02-08T03:04:03Z

This issue worries me: #10385 - something changed in closure compiler it seems, causing that project to break.

But perhaps that doesn't need to block this. Worst case we can disable minification inside HTML files if problems occur (which is a risk I wouldn't suggest for core minification of the main JS).

sbc100 · 2020-02-08T03:09:18Z

Right, that issue seems to be related to the closure upgrade rather than that fact that we used npm to install it. Good news that we can probably go ahead and land this when its ready.

juj · 2020-02-08T17:25:27Z

I don't think those errors are related to Closure update, but something has changed around __syscall3 function.

* Complete migration of stack overflow checks to library function in MINIMAL_RUNTIME, and add support for --separate-asm, MODULARIZE and ALLOW_MEMORY_GROWTH with MINIMAL_RUNTIME. * Use acorn-optimizer to minify <script> tags in .html code (TODO: Migrate to a better minifier, but needs one installed alongside the repository) * Remove manual JS code minification in HTML5, to use html-minifier-terser in PR emscripten-core#9990. * Update minimal_runtime test

juj added a commit to juj/emscripten that referenced this pull request Dec 8, 2019

Remove manual JS code minification in HTML5, to use html-minifier-ter…

e6789bc

…ser in PR emscripten-core#9990.

juj mentioned this pull request Dec 8, 2019

More MINIMAL_RUNTIME #9974

Merged

sbc100 reviewed Dec 9, 2019

View reviewed changes

package.json Show resolved Hide resolved

juj mentioned this pull request Dec 9, 2019

Migrate Closure to be located via npm install #9989

Merged

juj force-pushed the html-minifier-terser branch 3 times, most recently from cf3fd29 to c31dd42 Compare December 9, 2019 09:31

juj mentioned this pull request Dec 10, 2019

Add npm install step as post-build to Emscripten emscripten-core/emsdk#404

Merged

juj added a commit to juj/emscripten that referenced this pull request Dec 11, 2019

Remove manual JS code minification in HTML5, to use html-minifier-ter…

956ea7c

…ser in PR emscripten-core#9990.

juj changed the base branch from incoming to master January 14, 2020 18:57

juj mentioned this pull request Jan 22, 2020

JS fallback when Wasm is not supported #10118

Merged

juj force-pushed the html-minifier-terser branch 3 times, most recently from 3595e2f to efb8d87 Compare January 26, 2020 08:16

juj force-pushed the html-minifier-terser branch 2 times, most recently from cfcac9f to f990de3 Compare January 26, 2020 16:05

sbc100 approved these changes Jan 27, 2020

View reviewed changes

emcc.py Outdated Show resolved Hide resolved

emcc.py Outdated Show resolved Hide resolved

juj force-pushed the html-minifier-terser branch 4 times, most recently from 1de47f4 to ff6bb27 Compare February 1, 2020 17:29

juj added 5 commits February 6, 2020 19:32

html-minifier was buggy in minifying JS code, and author decided to d…

9a8041b

…rop support for minifying JS code in it altogether, so migrate to using html-minifier-terser that can minify <script> content inside HTML files.

Add env_with_node_in_path()

4059169

Update minimal runtime code sizes

5b0d130

update test sizes

26ec2e2

Fix typo

3cf4a65

juj force-pushed the html-minifier-terser branch from ff6bb27 to 3cf4a65 Compare February 6, 2020 17:33

Update minimal runtime code size test

910019b

sbc100 reviewed Feb 7, 2020

View reviewed changes

tools/shared.py Show resolved Hide resolved

Use env_with_node_in_path() with Closure

f02717d

juj force-pushed the html-minifier-terser branch from 9e107bd to f02717d Compare February 8, 2020 17:28

juj merged commit 479300e into emscripten-core:master Feb 8, 2020

Conversation

juj commented Dec 8, 2019

Uh oh!

juj commented Dec 8, 2019

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

juj commented Dec 9, 2019

Uh oh!

juj commented Dec 9, 2019

Uh oh!

juj commented Jan 26, 2020

Uh oh!

juj commented Jan 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sbc100 commented Jan 27, 2020

Uh oh!

juj commented Jan 28, 2020

Uh oh!

sbc100 commented Jan 28, 2020

Uh oh!

juj commented Feb 3, 2020

Uh oh!

sbc100 commented Feb 3, 2020

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sbc100 commented Feb 7, 2020

Uh oh!

kripken commented Feb 8, 2020

Uh oh!

sbc100 commented Feb 8, 2020

Uh oh!

juj commented Feb 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

juj commented Jan 26, 2020 •

edited

Loading