Add Closure compiler by JoviDeCroock · Pull Request #569 · developit/microbundle

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

 								},
 							},
 							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

+		.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


 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

 //# 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


 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


 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

 	--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

-							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

+		.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

 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


 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


 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


 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


 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


 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


 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 ForsakenHarmony, developit, 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

Conversation

JoviDeCroock commented Feb 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agilgur5 Feb 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoviDeCroock Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ForsakenHarmony commented Feb 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoviDeCroock commented Feb 18, 2020

Uh oh!

kristoferbaxter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ForsakenHarmony commented Mar 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

agilgur5 Feb 22, 2020 •

edited

Loading

JoviDeCroock Feb 18, 2020 •

edited

Loading