Compact output mode #2151

guybedford · 2018-04-22T21:32:45Z

This implements a compact output mode for Rollup wrappers and injections.

It doesn't remove whitespace in user code, only for the wrappers etc in the Rollup generation itself.

It will be interesting to see where this goes - perhaps whitespace removal in Rollup itself would be useful, perhaps it won't be great for performance, it's hard to tell.

But this gives some starting point that when combined with translate functions doing per-module minification gives an overall minified output that looks pretty good.

wa-Nadoo · 2018-04-24T21:57:09Z

test/form/samples/compact/_expected/iife.js

@@ -0,0 +1,3 @@
+varfoo=(function(x){'use strict';x=x&&x.hasOwnProperty('default')?x['default']:x;function foo () {


varfoo seems as a bug

lukastaegert

An interesting feature though I am not sure how much it will actually be used. There are a few things I would love to see tested + some other minor comments.

lukastaegert · 2018-05-05T11:58:56Z

src/ast/variables/NamespaceVariable.ts

@@ -47,11 +47,14 @@ export default class NamespaceVariable extends Variable {
 	}

 	renderBlock(options: RenderOptions) {
+		const _ = options.compact ? '' : ' ';
+		const nl = options.compact ? '' : '\n';


Just as t represents a \t, maybe this should actually be an n (You probably know I am not a huge fan of abbreviated variable names but this way it is actually somewhat consistent).

Also, even though it is not 100% certain it will be used, it would help readability to define t here as well and replace other usages of options.indent.

lukastaegert · 2018-05-05T12:02:28Z

src/ast/variables/NamespaceVariable.ts

@@ -61,27 +64,28 @@ export default class NamespaceVariable extends Variable {
 		const name = this.getName();

 		const callee = options.freeze
-			? `/*#__PURE__*/${options.legacy ? `(Object.freeze || Object)` : `Object.freeze`}`
+			? `${options.compact ? '' : '/*#__PURE__*/'}${


I do not think compact output should remove this annotation as this would mean that this is not only a visual change but it removes functionality, at least for some users, which I do not think is the intention here.

lukastaegert · 2018-05-05T12:52:25Z

src/Chunk.ts

-		if (addons.banner) magicString.prepend(addons.banner + '\n');
-		if (addons.footer) magicString.append('\n' + addons.footer);
+		if (addons.banner) magicString.prepend(addons.banner + nl);
+		if (addons.footer) magicString.append(nl + addons.footer);


I am not sure if NOT adding new lines after banners or before footers will not break things for some i.e. if the banner is a line-comment for headers or specific types of code for footers. Maybe addons.ts should take care there is always a line-break?

Good catch.

lukastaegert · 2018-05-05T12:53:29Z

src/Chunk.ts

-	private prepareDynamicImports({ format }: OutputOptions) {
+	private prepareDynamicImports({ format, compact }: OutputOptions) {
+		const _ = compact ? '' : ' ';
+		const nl = compact ? '' : '\n';


Again I would just call this n. Also, I would very much love to see a test for dynamic imports in compact mode.

lukastaegert · 2018-05-05T13:08:12Z

src/ast/variables/NamespaceVariable.ts

-				options.indent
-			}Object.defineProperty(${name}, 'toString', { value: function () { return '[object Module]' } });
-${callee}(${name});`;
+			const t = options.indent;


BTW is there a reason why this is not merged with the previous if statement?

guybedford · 2018-05-06T17:40:55Z

Thanks for the review, I've updated the PR.

lukastaegert

Thanks for the updates from the initial review. There are a few more things I stumbled upon which is mainly wrong code in the form test that went unnoticed. Maybe we can make this test a function test as well?

Also I would still like to see a test involving dynamic imports as the compactification of the dynamic import mechanism is still completely untested.

lukastaegert · 2018-05-11T14:24:54Z

src/Chunk.ts

-	private prepareDynamicImports({ format }: OutputOptions) {
+	private prepareDynamicImports({ format, compact }: OutputOptions) {
+		const _ = compact ? '' : ' ';
+		const n = compact ? '' : '\n';


This one is actually unused

lukastaegert · 2018-05-11T14:31:26Z

test/form/samples/compact/_expected/amd.js

@@ -0,0 +1,4 @@
+define(['external'],function(x){'use strict';x=x&&x.hasOwnProperty('default')?x['default']:x;var self = {get default(){returnfoo$$1}};if(typeof Symbol!=='undefined'&&Symbol.toStringTag)Object.defineProperty(self,Symbol.toStringTag,{value:'Module'});elseObject.defineProperty(self,'toString',{value:function(){return'[object Module]';}});/*#__PURE__*/Object.freeze(self);console.log(self);


Now this is why I think we need functional tests and should not stop writing them! There are at least two things wrong here that a functional test could probably have found but a human easily overlooks:

space missing in returnfoo$$1

space missing in elseObject

Well caught - will add these.

lukastaegert · 2018-05-11T14:35:50Z

test/form/samples/compact/_expected/amd.js

+define(['external'],function(x){'use strict';x=x&&x.hasOwnProperty('default')?x['default']:x;var self = {get default(){returnfoo$$1}};if(typeof Symbol!=='undefined'&&Symbol.toStringTag)Object.defineProperty(self,Symbol.toStringTag,{value:'Module'});elseObject.defineProperty(self,'toString',{value:function(){return'[object Module]';}});/*#__PURE__*/Object.freeze(self);console.log(self);
+function foo$$1 () {
+	console.log( x );
+}return foo$$1;});


Also it would be nice to separate the user code from the wrapper via new-lines. Not only is it more consistent with the concept of only compactifying the auto-generated code, it will also break down if the user's code ends with a line comment (would not hurt to add this to the test as well)

Good point. Let me rather add the ability to detect and remove trailing line comments.

The intention here is that a plugin can perform minification in the transform hook to get an overall minification build running over per-module minify workers.

But trailing line-comments could also be a purposefully placed footer comment. It might be ok to remove it in some situations but we actually have logic to NOT do this as it is still one goal for rollup to preserve all comments a user added as long as we think they do not describe removed code. If a minifier is doing its thing, it will remove those comments anyway so the presence of such a comment would signal that minification was NOT performed and is NOT the intended goal here.

And line comments are just an example. If you throw a line such as const x = 3 return foo; into acorn, it will tell you it is invalid syntax. So you also need to make sure there is a semicolon. Etc. etc.

All in all I am not happy about this approach and would prefer the line-break.

On the other hand, there is no reason to keep the semicolon after return foo$$1

Oh, reading the actual code you wrote, you actually DO preserve the trailing comments. So I am cool with that.

Nevertheless, we should also detect for the situation where the last line is non-empty, does not contain a line-comment and where the last non-whitespace character is not a semicolon in which cases I would suggest to append a semicolon instead.

lukastaegert · 2018-05-11T14:41:07Z

src/utils/addons.ts

-		collectAddon(graph, options.banner, 'banner'),
-		collectAddon(graph, options.footer, 'footer'),
+		collectAddon(graph, options.banner, 'banner', '\n'),
+		collectAddon(graph, options.footer, 'footer', '\n'),


This looks much safer 👍

lukastaegert · 2018-05-11T14:44:27Z

test/form/samples/compact/main.js

@@ -0,0 +1,6 @@
+import x from 'external';
+import * as self from './main.js';


Nice adding this!

guybedford · 2018-05-13T18:34:59Z

Thanks again for the thorough review here and catching the edge cases. I've addressed the changes and included the functional tests you mentioned.

For checking a last line comment on a source I created Rich-Harris/magic-string#142 and added that check here, so that should be merged first.

lukastaegert

Thanks for checking my comments, I am nearly good with this except the issue I outlined in some places where I would prefer to see a semicolon. Might be worth checking out if there could be issues even in the non-compact case that we might fix here as well. #2110 is a prime candidate for this as the issue manifests due to the module concatenation.

lukastaegert · 2018-05-14T06:57:35Z

src/Chunk.ts

@@ -802,6 +808,7 @@ export default class Chunk {
 		for (const module of this.orderedModules) {
 			const source = module.render(renderOptions);
 			source.trim();
+			if (options.compact && source.lastLine().indexOf('//') !== -1) source.append('\n');


As already stated, we should also add a semi-colon if there is neither a line-comment nor a trailing semi-colon.

lukastaegert · 2018-05-14T07:03:22Z

test/chunking-form/samples/chunking-compact/_expected/amd/chunk-1e9855f7.js

@@ -0,0 +1,6 @@
+define(['exports'],function(exports){'use strict';function fn () {
+  console.log('lib2 fn');
+}function fn$1 () {


This only works because function declarations do not require semicolons to be on the same line as other statements. The same does not hold for e.g. other declarations—

const x =3function y(){}

would NOT be valid code, even if you add a space in between. A semicolon would be the proper solution here.

Note that in the const x = 3 case, the VariableDeclaration render function will ensure a trailing semicolon already for us.

Oh, you are right! For declarations and expression statements, this is taken care of 👍

lukastaegert · 2018-05-14T07:05:44Z

test/chunking-form/samples/chunking-compact/_expected/amd/chunk-1e9855f7.js

+}function fn$1 () {
+  fn();
+  console.log('dep2 fn');
+}exports.a=fn$1;});


and again we should consider adding semicolons even if it means adding empty statements in some situations. Alternatively, line-breaks might work as well but would not solve issues where the succeeding code starts with an opening bracket.

lukastaegert · 2018-05-14T07:07:18Z

test/function/samples/compact/_config.js

@@ -0,0 +1,23 @@
+module.exports = {


👍 for adding this test

guybedford · 2018-05-14T10:59:28Z

I've added a fix here for #2110.

I think we shouldn't add a semicolon for the compact case actually - since these cases are already handled by the renderer itself in the individual node renders, which seems exactly how this should be handled to me.

lukastaegert

Looks good! Now we just need the magic-string PR to be released.

guybedford · 2018-05-15T19:15:13Z

With the magic string update, the tests are now passing here.

@lukastaegert happy to keep this experimental for a while, and just let me know if you want to rename to experimentalCompact for example.

tivac · 2018-05-15T20:43:24Z

What's the advantage to having rollup do this itself (though only for the rollup-added wrapping code) as opposed to doing whole-app compaction once the bundle is generated?

guybedford · 2018-05-18T11:56:56Z

@tivac it's an experimental step along the path to turning Rollup into a minifier. For example, mangling is an easy feature for Rollup to add, which when combined with this gets us there. I'm then experimenting with per-source minification for a fast build workflow. Has to be weighed up against whole-graph minification optimizations, but so far there doesn't seem to be much loss. 2/3 build time feels like a win to me so far, but as I say it's experimental.

lukastaegert · 2018-05-22T06:48:21Z

@guybedford I do not think we need an experimental option for this. Even though the exact results of this option may change, I would expect it to remain valid at least for a while.

wa-Nadoo reviewed Apr 24, 2018

View reviewed changes

guybedford requested a review from lukastaegert April 29, 2018 10:01

guybedford force-pushed the compact branch 2 times, most recently from aa72b97 to b42bbba Compare April 30, 2018 20:30

lukastaegert reviewed May 5, 2018

View reviewed changes

lukastaegert requested changes May 11, 2018

View reviewed changes

guybedford mentioned this pull request May 13, 2018

Support .lastLine() Rich-Harris/magic-string#142

Merged

guybedford force-pushed the compact branch from f89d2ba to 12eb4d6 Compare May 13, 2018 18:34

lukastaegert requested changes May 14, 2018

View reviewed changes

guybedford force-pushed the compact branch from 2acb378 to d9ac858 Compare May 14, 2018 10:57

lukastaegert approved these changes May 15, 2018

View reviewed changes

guybedford force-pushed the compact branch from d9ac858 to 4766e15 Compare May 15, 2018 10:29

guybedford added 11 commits May 22, 2018 08:55

implement compact output option

e1599fb

include depenency in compact test

b16d18e

compact cjs test fix

2065aa4

update cli test cases

bccb7aa

ensure compact esmodule export

d0640bd

remove unnecessary semicolon in cjs output

0af1a6f

fix missing space

0d24ab8

mangle chunk boundaries in compact mode

0ecc0ea

pr feedback

099c4f7

compact feedback

7d42762

ensure semicolon between compact sources

0e65bec

guybedford added 3 commits May 22, 2018 08:55

handling trailing module statement asi cases

a6d1978

simplify asi handling due to render delegations

03efe10

rebase corrections

752447d

lukastaegert force-pushed the compact branch from bd2dfcc to 752447d Compare May 22, 2018 06:58

lukastaegert merged commit 752447d into master May 22, 2018

lukastaegert added the fixed pending release label May 22, 2018

lukastaegert deleted the compact branch May 22, 2018 07:00

lukastaegert mentioned this pull request May 23, 2018

Add test for #2110 #2209

Merged

lukastaegert added this to the 0.60.0 milestone May 24, 2018

lukastaegert removed the fixed pending release label Jun 6, 2018

		@@ -0,0 +1,3 @@
		varfoo=(function(x){'use strict';x=x&&x.hasOwnProperty('default')?x['default']:x;function foo () {

		@@ -0,0 +1,4 @@
		define(['external'],function(x){'use strict';x=x&&x.hasOwnProperty('default')?x['default']:x;var self = {get default(){returnfoo$$1}};if(typeof Symbol!=='undefined'&&Symbol.toStringTag)Object.defineProperty(self,Symbol.toStringTag,{value:'Module'});elseObject.defineProperty(self,'toString',{value:function(){return'[object Module]';}});/#__PURE__/Object.freeze(self);console.log(self);

		@@ -0,0 +1,6 @@
		import x from 'external';
		import * as self from './main.js';

Compact output mode #2151

Compact output mode #2151

Conversation

guybedford commented Apr 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukastaegert 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

guybedford commented May 6, 2018

lukastaegert 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

lukastaegert May 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guybedford commented May 13, 2018

lukastaegert 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

guybedford commented May 14, 2018

lukastaegert left a comment

Choose a reason for hiding this comment

guybedford commented May 15, 2018

tivac commented May 15, 2018

guybedford commented May 18, 2018

lukastaegert commented May 22, 2018

lukastaegert May 14, 2018 •

edited