Add Closure compiler #569

JoviDeCroock · 2020-02-17T18:01:30Z

Adds the default true option to use the closure compiler and gives the option to opt-out and go for terser instead

ForsakenHarmony · 2020-02-18T22:14:19Z

src/index.js

@@ -555,6 +556,7 @@ function createConfig(options, entry, format, writeMeta) {
 								},
 							},
 							tsconfig: options.tsconfig,
+							objectHashIgnoreUnknownHack: !!options.closure,


what's that?

Without it async functions won't work with the ts plugin when combined with closure

ezolenko/rollup-plugin-typescript2#105

Just randomly saw this linked, but per the last comment there (from me a week before this PR was made), it's no longer necessary if you update to v0.26.0+ .

And per the other comments / former docs, if you are using the objectHashIgnoreUnknownHack option, you should really use it with clean: true as otherwise it can cause caching issues. That means it's slower as there's no cache, which is why I pushed to get the root cause fixed

ForsakenHarmony · 2020-02-18T22:15:37Z

src/prog.js

+		.option(
+			'--closure',
+			'Use the closure-compiler for minifying instead of terser',
+			true,


should this be the default?

Well it was a proposition by @kristoferbaxter

Since this is a tool people universally expect to pass options to (due to no config), I think we should start out by merging this with the default still as Terser. We can quickly cut over to Closure as a new default once it's sat on master for a version cycle. I just don't want landing this PR and getting closure support published to necessitate a major version change all at once.

That’s a fair compromise. Let’s make this an option (not default) to land, and iterate on output.

To become the default I think we need to support the custom mangle format microbundle uses (uglify’s native format).

Also, the newlines should be addressed for consistency. (This work should likely be part of the rollup closure plugin).

ForsakenHarmony · 2020-02-18T22:35:10Z

Also does this significantly increase build times?

ForsakenHarmony · 2020-02-18T22:37:33Z

test/__snapshots__/index.test.js.snap

@@ -1176,7 +1177,7 @@ exports[`fixtures build css-modules--string with microbundle 3`] = `
 `;

 exports[`fixtures build css-modules--string with microbundle 4`] = `
-"export default function(){var a=document.createElement(\\"div\\");return a.className=\\"_contains_this_0a8c24df242c2cd708036873307aea94 _contains_this_81567d0efc15a456670452d3277e1a68\\",a}
+"var b={test_class_that_should_be_scoped:\\"_contains_this_81567d0efc15a456670452d3277e1a68\\"},c={scoped_class:\\"_contains_this_0a8c24df242c2cd708036873307aea94\\"};export default function(){var a=document.createElement(\\"div\\");a.className=c.scoped_class+\\" \\"+b.test_class_that_should_be_scoped;return a}


these are a bit disappointing I guess

Let's file an issue for this, I'm curious what the input was here.

It's an object coming from an import - I'm thinking closure can't inline because its crossing a module boundary?

@kristoferbaxter maybe cross-module code motion is only enabled for ADVANCED_OPTIMIZATIONS?

Correct @developit

JoviDeCroock · 2020-02-18T23:43:34Z

@ForsakenHarmony It does seem equally as fast as terser from what I'm seeing

kristoferbaxter

Looks like mangle property files are not being respected. This would be a feature request for the rollup plugin or (preferably) a custom babel pass before invoking either minifier.

kristoferbaxter · 2020-02-17T18:58:03Z

test/__snapshots__/index.test.js.snap

 //# sourceMappingURL=basic-lib.esm.js.map
 "
 `;

 exports[`fixtures build basic with microbundle 4`] = `
-"var r=function(){try{for(var r=arguments.length,e=new Array(r),n=0;n<r;n++)e[n]=arguments[n];return Promise.resolve(e.reduce(function(r,e){return r+e},0))}catch(r){return Promise.reject(r)}};module.exports=function(){for(var e=arguments.length,n=new Array(e),t=0;t<e;t++)n[t]=arguments[t];try{return Promise.resolve(r.apply(void 0,n)).then(function(e){return Promise.resolve(r.apply(void 0,n)).then(function(r){return[e,r]})})}catch(r){return Promise.reject(r)}};
+"'use strict';var two=function(){try{for(var c=arguments.length,b=Array(c),a=0;a<c;a++)b[a]=arguments[a];return Promise.resolve(b.reduce(function(a,b){return a+b},0))}catch(d){return Promise.reject(d)}},index=function(){for(var c=arguments.length,b=Array(c),a=0;a<c;a++)b[a]=arguments[a];try{return Promise.resolve(two.apply(void 0,b)).then(function(a){return Promise.resolve(two.apply(void 0,b)).then(function(b){return[a,b]})})}catch(d){return Promise.reject(d)}};module.exports=index;


Perhaps we should add a config option to rollup-plugin-closure-compiler to omit 'use strict' directives when requested.

kristoferbaxter · 2020-02-18T23:49:24Z

test/__snapshots__/index.test.js.snap

@@ -1176,7 +1177,7 @@ exports[`fixtures build css-modules--string with microbundle 3`] = `
 `;

 exports[`fixtures build css-modules--string with microbundle 4`] = `
-"export default function(){var a=document.createElement(\\"div\\");return a.className=\\"_contains_this_0a8c24df242c2cd708036873307aea94 _contains_this_81567d0efc15a456670452d3277e1a68\\",a}
+"var b={test_class_that_should_be_scoped:\\"_contains_this_81567d0efc15a456670452d3277e1a68\\"},c={scoped_class:\\"_contains_this_0a8c24df242c2cd708036873307aea94\\"};export default function(){var a=document.createElement(\\"div\\");a.className=c.scoped_class+\\" \\"+b.test_class_that_should_be_scoped;return a}


Let's file an issue for this, I'm curious what the input was here.

kristoferbaxter · 2020-02-19T00:02:55Z

test/__snapshots__/index.test.js.snap

 198 B: mangle-json-file.umd.js.gz
 164 B: mangle-json-file.umd.js.br"
 `;

 exports[`fixtures build mangle-json-file with microbundle 2`] = `6`;

 exports[`fixtures build mangle-json-file with microbundle 3`] = `
-"var o={prop1:1,__p2:2};export default function(){return console.log(o.prop1),console.log(o.__p2),o}
+"var a={prop1:1,_prop2:2};export default function(){console.log(a.prop1);console.log(a._prop2);return a}


This looks like a bug or issue to resolve before merging, the mangled value isn't being used.

https://github.com/developit/microbundle/wiki/mangle.json#for-manglejson-implementors

developit · 2020-02-19T01:17:42Z

README.md

@@ -173,6 +173,7 @@ Options
 	--tsconfig       Specify the path to a custom tsconfig.json
 	--css-modules    Configures .css to be treated as modules (default: null)
 	-h, --help       Displays this message
+	--closure				 Specify if using the closure-compiler, when false microbundle will use terser instead (default: true)


If the default is to use closure, the option should be --terser. See other comment for discussion of whether default is correct here.

developit · 2020-02-19T01:19:02Z

src/index.js

-							mangle: Object.assign({}, minifyOptions.mangle || {}),
-							nameCache,
-						}),
+						options.closure && (format === 'es' || modern)


We should clarify in the docs/help that Terser is always used for cjs,iife,umd formats, regardless of the --closure option.

developit · 2020-02-19T01:20:43Z

src/prog.js

+		.option(
+			'--closure',
+			'Use the closure-compiler for minifying instead of terser',
+			true,


Since this is a tool people universally expect to pass options to (due to no config), I think we should start out by merging this with the default still as Terser. We can quickly cut over to Closure as a new default once it's sat on master for a version cycle. I just don't want landing this PR and getting closure support published to necessitate a major version change all at once.

developit · 2020-02-19T01:21:21Z

test/__snapshots__/index.test.js.snap

@@ -20,16 +20,16 @@ alias
 Build \\"aliasMapping\\" to dist:
 62 B: alias-mapping.js.gz
 46 B: alias-mapping.js.br
-62 B: alias-mapping.esm.js.gz
-46 B: alias-mapping.esm.js.br
+61 B: alias-mapping.esm.js.gz


as per side-discussion, this size decrease is due to custom property name mappings being ignored.

developit · 2020-02-19T01:24:14Z

test/__snapshots__/index.test.js.snap

@@ -968,7 +968,8 @@ Build \\"classDecoratorsTs\\" to dist:
 exports[`fixtures build class-decorators-ts with microbundle 2`] = `7`;

 exports[`fixtures build class-decorators-ts with microbundle 3`] = `
-"var e=function(){function e(e){this.greeting=e}return e.prototype.greet=function(){return\\"Hello, \\"+this.greeting},e}(),t=new(e=function(e,t,r,n){var o,c=arguments.length,l=c<3?t:null===n?n=Object.getOwnPropertyDescriptor(t,r):n;if(\\"object\\"==typeof Reflect&&\\"function\\"==typeof Reflect.decorate)l=Reflect.decorate(e,t,r,n);else for(var f=e.length-1;f>=0;f--)(o=e[f])&&(l=(c<3?o(l):c>3?o(t,r,l):o(t,r))||l);return c>3&&l&&Object.defineProperty(t,r,l),l}([function(e){Object.seal(e),Object.seal(e.prototype)}],e))(\\"Hello World\\");export default t;
+"var h=function(){function a(a){this.greeting=a}a.prototype.greet=function(){return\\"Hello, \\"+this.greeting};return a}();


newline here seems odd...

developit · 2020-02-19T01:26:21Z

test/__snapshots__/index.test.js.snap

@@ -1176,7 +1177,7 @@ exports[`fixtures build css-modules--string with microbundle 3`] = `
 `;

 exports[`fixtures build css-modules--string with microbundle 4`] = `
-"export default function(){var a=document.createElement(\\"div\\");return a.className=\\"_contains_this_0a8c24df242c2cd708036873307aea94 _contains_this_81567d0efc15a456670452d3277e1a68\\",a}
+"var b={test_class_that_should_be_scoped:\\"_contains_this_81567d0efc15a456670452d3277e1a68\\"},c={scoped_class:\\"_contains_this_0a8c24df242c2cd708036873307aea94\\"};export default function(){var a=document.createElement(\\"div\\");a.className=c.scoped_class+\\" \\"+b.test_class_that_should_be_scoped;return a}


It's an object coming from an import - I'm thinking closure can't inline because its crossing a module boundary?

@kristoferbaxter maybe cross-module code motion is only enabled for ADVANCED_OPTIMIZATIONS?

developit · 2020-02-19T01:27:35Z

test/__snapshots__/index.test.js.snap

 170 B: default-named.umd.js.gz
 124 B: default-named.umd.js.br"
 `;

 exports[`fixtures build default-named with microbundle 2`] = `6`;

 exports[`fixtures build default-named with microbundle 3`] = `
-"var t=42;export default function(){}export{t as foo};
+"export default function(){};var foo=42;export{foo}


This is a fantastic output.

developit · 2020-02-19T01:28:57Z

test/__snapshots__/index.test.js.snap

 1072 B: esnext-ts.umd.js.gz
 975 B: esnext-ts.umd.js.br"
 `;

 exports[`fixtures build esnext-ts with microbundle 2`] = `7`;

 exports[`fixtures build esnext-ts with microbundle 3`] = `
-"var n=function(){function n(){}return n.prototype.then=function(r,e){var o=new n,i=this.s;if(i){var u=1&i?r:e;if(u){try{t(o,1,u(this.v))}catch(n){t(o,2,n)}return o}return this}return this.o=function(n){try{var i=n.v;1&n.s?t(o,1,r?r(i):i):e?t(o,1,e(i)):t(o,2,i)}catch(n){t(o,2,n)}},o},n}();function t(r,e,o){if(!r.s){if(o instanceof n){if(!o.s)return void(o.o=t.bind(null,r,e));1&e&&(e=o.s),o=o.v}if(o&&o.then)return void o.then(t.bind(null,r,e),t.bind(null,r,2));r.s=e,r.v=o;var i=r.o;i&&i(r)}}function r(t){return t instanceof n&&1&t.s}function e(n,t){try{var r=n()}catch(n){return t(!0,n)}return r&&r.then?r.then(t.bind(null,!1),t.bind(null,!0)):t(!1,r)}\\"undefined\\"!=typeof Symbol&&(Symbol.iterator||(Symbol.iterator=Symbol(\\"Symbol.iterator\\"))),\\"undefined\\"!=typeof Symbol&&(Symbol.asyncIterator||(Symbol.asyncIterator=Symbol(\\"Symbol.asyncIterator\\")));var o=function(){try{var o,i,u,f,h=[],c=!0,a=!1,l=e(function(){return function(e,f){try{var a=function(){o=function(n){var t;if(\\"undefined\\"!=typeof Symbol){if(Symbol.asyncIterator&&null!=(t=n[Symbol.asyncIterator]))return t.call(n);if(Symbol.iterator&&null!=(t=n[Symbol.iterator]))return t.call(n)}throw new TypeError(\\"Object is not async iterable\\")}([1,2]);var e=function(e,o,i){for(var u;;){var f=e();if(r(f)&&(f=f.v),!f)return h;if(f.then){u=0;break}var h=i();if(h&&h.then){if(!r(h)){u=1;break}h=h.s}if(o){var c=o();if(c&&c.then&&!r(c)){u=2;break}}}var a=new n,l=t.bind(null,a,2);return(0===u?f.then(s):1===u?h.then(v):c.then(y)).then(void 0,l),a;function v(n){h=n;do{if(o&&(c=o())&&c.then&&!r(c))return void c.then(y).then(void 0,l);if(!(f=e())||r(f)&&!f.v)return void t(a,1,h);if(f.then)return void f.then(s).then(void 0,l);r(h=i())&&(h=h.v)}while(!h||!h.then);h.then(v).then(void 0,l)}function s(n){n?(h=i())&&h.then?h.then(v).then(void 0,l):v(h):t(a,1,h)}function y(){(f=e())?f.then?f.then(s).then(void 0,l):s(f):t(a,1,h)}}(function(){return!!Promise.resolve(o.next()).then(function(n){return c=i.done,i=n,Promise.resolve(i.value).then(function(n){return u=n,!c})})},function(){return!!(c=!0)},function(){h.push(u)});if(e&&e.then)return e.then(function(){})}()}catch(n){return f(n)}return a&&a.then?a.then(void 0,f):a}(0,function(n){a=!0,f=n})},function(n,t){function r(r){if(n)throw t;return t}var i=e(function(){var n=function(){if(!c&&null!=o.return)return Promise.resolve(o.return()).then(function(){})}();if(n&&n.then)return n.then(function(){})},function(n,t){if(a)throw f;if(n)throw t;return t});return i&&i.then?i.then(r):r()});return Promise.resolve(l&&l.then?l.then(function(n){return h}):h)}catch(n){return Promise.reject(n)}};o().then(console.log);export default o;
+"function h(a){if(\\"undefined\\"!==typeof Symbol){if(Symbol.asyncIterator){var b=a[Symbol.asyncIterator];if(null!=b)return b.call(a)}if(Symbol.iterator&&(b=a[Symbol.iterator],null!=b))return b.call(a)}throw new TypeError(\\"Object is not async iterable\\");}


Is there a default line length configuration in Closure or something? It's strange there would be line breaks in the output at all.

developit · 2020-02-19T01:29:45Z

test/__snapshots__/index.test.js.snap

 198 B: mangle-json-file.umd.js.gz
 164 B: mangle-json-file.umd.js.br"
 `;

 exports[`fixtures build mangle-json-file with microbundle 2`] = `6`;

 exports[`fixtures build mangle-json-file with microbundle 3`] = `
-"var o={prop1:1,__p2:2};export default function(){return console.log(o.prop1),console.log(o.__p2),o}
+"var a={prop1:1,_prop2:2};export default function(){console.log(a.prop1);console.log(a._prop2);return a}


https://github.com/developit/microbundle/wiki/mangle.json#for-manglejson-implementors

JoviDeCroock · 2020-03-01T11:10:46Z

test/__snapshots__/index.test.js.snap

@@ -1225,7 +1226,7 @@ exports[`fixtures build css-modules--true with microbundle 3`] = `
 `;

 exports[`fixtures build css-modules--true with microbundle 4`] = `
-"export default function(){var e=document.createElement(\\"div\\");return e.className=\\"_2kWDE _1E6DU\\",e}
+"var b={test_class_that_should_be_scoped:\\"_1E6DU\\"},c={scoped_class:\\"_2kWDE\\"};export default function(){var a=document.createElement(\\"div\\");a.className=c.scoped_class+\\" \\"+b.test_class_that_should_be_scoped;return a}


@kristoferbaxter this one is quite the downgrade, I'll file an issue for this

ForsakenHarmony · 2020-03-02T13:27:02Z

we can merge this now with terser still being the default and decide later if we want to make closure the default

JoviDeCroock added 8 commits February 17, 2020 18:57

add closure compiler option and implement it

e3534b8

update readme

efecc9c

use the correct dep

35d6b7c

update snapshots

73993d1

fix typescript compilation for closure

53f7523

only use in es and modern mode

2520202

update to new version

fdb38d3

no strict directive

79638e8

JoviDeCroock requested review from developit, ForsakenHarmony, kristoferbaxter and marvinhagemeister February 17, 2020 23:50

JoviDeCroock added 3 commits February 18, 2020 00:50

Merge branch 'master' into closure-compiler

986cf9c

remove node_modules from snapshot

d08fa57

updateSnapshot didn't apply last time

ee6f99b

ForsakenHarmony reviewed Feb 18, 2020

View reviewed changes

kristoferbaxter reviewed Feb 19, 2020

View reviewed changes

developit reviewed Feb 19, 2020

View reviewed changes

JoviDeCroock added 2 commits February 23, 2020 19:06

remove the objectHashIgnoreUnknownHack option

71ca79a

Merge branch 'master' into closure-compiler

c48ee08

agilgur5 mentioned this pull request Feb 28, 2020

(feat): closure compiler for minification jaredpalmer/tsdx#525

Closed

Merge branch 'master' into closure-compiler

054c9d3

JoviDeCroock commented Mar 1, 2020

View reviewed changes

JoviDeCroock closed this Apr 23, 2021

JoviDeCroock deleted the closure-compiler branch April 23, 2021 10:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Closure compiler #569

Add Closure compiler #569

JoviDeCroock commented Feb 17, 2020

ForsakenHarmony Feb 18, 2020

JoviDeCroock Feb 18, 2020

agilgur5 Feb 22, 2020 •

edited

ForsakenHarmony Feb 18, 2020

JoviDeCroock Feb 18, 2020 •

edited

developit Feb 19, 2020

kristoferbaxter Feb 23, 2020

ForsakenHarmony commented Feb 18, 2020

ForsakenHarmony Feb 18, 2020

kristoferbaxter Feb 18, 2020

developit Feb 19, 2020

kristoferbaxter Mar 2, 2020

JoviDeCroock commented Feb 18, 2020

kristoferbaxter left a comment

kristoferbaxter Feb 17, 2020

kristoferbaxter Feb 18, 2020

kristoferbaxter Feb 19, 2020

developit Feb 19, 2020

developit Feb 19, 2020

developit Feb 19, 2020

developit Feb 19, 2020

developit Feb 19, 2020

developit Feb 19, 2020

developit Feb 19, 2020

developit Feb 19, 2020

developit Feb 19, 2020

developit Feb 19, 2020

JoviDeCroock Mar 1, 2020

ForsakenHarmony commented Mar 2, 2020

Add Closure compiler #569

Add Closure compiler #569

Conversation

JoviDeCroock commented Feb 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agilgur5 Feb 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoviDeCroock Feb 18, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ForsakenHarmony commented Feb 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoviDeCroock commented Feb 18, 2020

kristoferbaxter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ForsakenHarmony commented Mar 2, 2020

agilgur5 Feb 22, 2020 •

edited

JoviDeCroock Feb 18, 2020 •

edited