Refine SystemJS export rendering by lukastaegert · Pull Request #4199 · rollup/rollup

lukastaegert · 2021-08-02T04:23:38Z

This PR contains:

Are tests included?

yes (bugfixes and features will not be merged without tests)
no

Breaking Changes?

yes (breaking changes will not be merged unless absolutely necessary)
no

List any relevant issue numbers:

Description

In SystemJS, we manually need to update export bindings whenever the underlying variable changes. This PR refines and unifies the code created by Rollup, often replacing IIFEs with more efficient comma expressions.

It also fixes an issue where using a postfix unary update operator applied to a variable with more than one export name would return the updated value instead of the original value: Old REPL, New REPL.

This will also make the code easier to maintain and help when generating ES2015+ output.

Here is an overview of the most important changes:

/* INPUT */
// ---
// Single export name
export let foo = 1;

// Assignment
// foo = 2
foo = 2;
console.log(foo = 2);

// foo += 2
foo += 2;
console.log(foo += 2);

// { foo } = obj
({ foo } = obj);
console.log({ foo } = obj);

// Update
// foo++
foo++;
console.log(foo++);
foo--;

// ++foo
++foo;
console.log(++foo);
--foo;

// ---
// Multiple export names
export let bar = 1;
export { bar as bar2 };

// Assignment
// bar = 2
bar = 2;
console.log(bar = 2);

// bar += 2
bar += 2;
console.log(bar += 2);

// { bar } = obj
({ bar } = obj);
console.log({ bar } = obj);

// Update
// bar++
bar++;
console.log(bar++);
bar--;

// ++bar
++bar;
console.log(++bar);
--bar;

/* PREVIOUS OUTPUT */
System.register('myBundle', [], function (exports) {
	'use strict';
	return {
		execute: function () {

			// ---
			// Single export name
			let foo = exports('foo', 1);

			// Assignment
			// foo = 2
			foo = exports('foo', 2);
			console.log(foo = exports('foo', 2));

			// foo += 2
			foo = exports('foo', foo + 2);
			console.log(foo = exports('foo', foo + 2));

			// { foo } = obj
			((function (v) { return exports({ foo: foo }), v; }({ foo } = obj)));
			console.log((function (v) { return exports({ foo: foo }), v; }({ foo } = obj)));

			// Update
			// foo++
			(exports('foo', foo + 1), foo++);
			console.log((exports('foo', foo + 1), foo++));
			(exports('foo', foo - 1), foo--);

			// ++foo
			exports('foo', ++foo);
			console.log(exports('foo', ++foo));
			exports('foo', --foo);

			// ---
			// Multiple export names
			let bar = function (v) { return exports({ bar: v, bar2: v }), v; }(1);

			// Assignment
			// bar = 2
			bar = function (v) { return exports({ bar: v, bar2: v }), v; }(2);
			console.log(bar = function (v) { return exports({ bar: v, bar2: v }), v; }(2));

			// bar += 2
			bar = function (v) { return exports({ bar: v, bar2: v }), v; }(bar + 2);
			console.log(bar = function (v) { return exports({ bar: v, bar2: v }), v; }(bar + 2));

			// { bar } = obj
			((function (v) { return exports({ bar: bar, bar2: bar }), v; }({ bar } = obj)));
			console.log((function (v) { return exports({ bar: bar, bar2: bar }), v; }({ bar } = obj)));

			// Update
			// bar++
			(function (v) { return exports({ bar: v, bar2: v }), v; }(++bar));
			console.log((function (v) { return exports({ bar: v, bar2: v }), v; }(++bar)));
			(function (v) { return exports({ bar: v, bar2: v }), v; }(--bar));

			// ++bar
			(++bar, exports({ bar: bar, bar2: bar }), bar);
			console.log((++bar, exports({ bar: bar, bar2: bar }), bar));
			(--bar, exports({ bar: bar, bar2: bar }), bar);

		}
	};
});

/* NEW OUTPUT */
System.register([], function (exports) {
	'use strict';
	return {
		execute: function () {

			// ---
			// Single export name
			let foo = exports('foo', 1);

			// Assignment
			// foo = 2
			exports('foo', foo = 2);
			console.log(exports('foo', foo = 2));

			// foo += 2
			exports('foo', foo += 2);
			console.log(exports('foo', foo += 2));

			// { foo } = obj
			(function (v) { return exports('foo', foo), v; }({ foo } = obj));
			console.log(function (v) { return exports('foo', foo), v; }({ foo } = obj));

			// Update
			// foo++
			exports('foo', foo + 1), foo++;
			console.log((exports('foo', foo + 1), foo++));
			exports('foo', foo - 1), foo--;

			// ++foo
			exports('foo', ++foo);
			console.log(exports('foo', ++foo));
			exports('foo', --foo);

			// ---
			// Multiple export names
			let bar = 1; exports({ bar: bar, bar2: bar });

			// Assignment
			// bar = 2
			bar = 2, exports({ bar: bar, bar2: bar }), bar;
			console.log((bar = 2, exports({ bar: bar, bar2: bar }), bar));

			// bar += 2
			bar += 2, exports({ bar: bar, bar2: bar }), bar;
			console.log((bar += 2, exports({ bar: bar, bar2: bar }), bar));

			// { bar } = obj
			(function (v) { return exports({ bar: bar, bar2: bar }), v; }({ bar } = obj));
			console.log(function (v) { return exports({ bar: bar, bar2: bar }), v; }({ bar } = obj));

			// Update
			// bar++
			exports({ bar: bar + 1, bar2: bar + 1 }), bar++;
			console.log((exports({ bar: bar + 1, bar2: bar + 1 }), bar++));
			exports({ bar: bar - 1, bar2: bar - 1 }), bar--;

			// ++bar
			++bar, exports({ bar: bar, bar2: bar }), bar;
			console.log((++bar, exports({ bar: bar, bar2: bar }), bar));
			--bar, exports({ bar: bar, bar2: bar }), bar;

		}
	};
});

github-actions · 2021-08-02T04:25:14Z

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install rollup/rollup#improve-systemjs-rendering

or load it into the REPL:
https://rollupjs.org/repl/?pr=4199

codecov · 2021-08-02T04:59:54Z

Codecov Report

Merging #4199 (45bafc2) into master (66d2d82) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4199      +/-   ##
==========================================
+ Coverage   98.34%   98.37%   +0.02%     
==========================================
  Files         202      202              
  Lines        7244     7254      +10     
  Branches     2123     2127       +4     
==========================================
+ Hits         7124     7136      +12     
  Misses         58       58              
+ Partials       62       60       -2

Impacted Files	Coverage Δ
src/Chunk.ts	`100.00% <ø> (ø)`
src/ast/nodes/AssignmentExpression.ts	`100.00% <100.00%> (ø)`
src/ast/nodes/ImportExpression.ts	`100.00% <100.00%> (ø)`
src/ast/nodes/UpdateExpression.ts	`100.00% <100.00%> (ø)`
src/ast/nodes/VariableDeclaration.ts	`98.05% <100.00%> (+0.18%)`	⬆️
src/finalisers/system.ts	`100.00% <100.00%> (+2.22%)`	⬆️
src/utils/systemJsRendering.ts	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66d2d82...45bafc2. Read the comment docs.

guybedford · 2021-08-02T15:29:56Z

+			var i, j; exports({ i: i, j: j });

-			var k, l;
+			var k, l; exports({ k: k, l: l });


As much as possible it is usually a good idea to bulk together calls to the exports({}) function. This is because when there are export * or reexports in play, there is a cascade of function calls to propagate the binding updates, so fewer cascades are better for performance as this can degrade on very exported graphs otherwise.

Undefined bindings are an exception, but this reduces quite a bit complexity. On the other hand, there is now much more grouping for declarations that contain more than one export, see below lines 22 and 24.

guybedford

Only did a brief review, but looks good to me.

lukastaegert force-pushed the improve-systemjs-rendering branch 2 times, most recently from 1086780 to d0aa307 Compare August 2, 2021 04:55

lukastaegert force-pushed the improve-systemjs-rendering branch from d0aa307 to 1d97348 Compare August 2, 2021 05:25

Refine SystemJS export rendering

52ce573

lukastaegert force-pushed the improve-systemjs-rendering branch from 1d97348 to 52ce573 Compare August 2, 2021 05:26

guybedford reviewed Aug 2, 2021

View reviewed changes

guybedford approved these changes Aug 2, 2021

View reviewed changes

Improve coverage

45bafc2

lukastaegert merged commit b34a411 into master Aug 5, 2021

lukastaegert deleted the improve-systemjs-rendering branch August 5, 2021 05:10

sync-by-unito Bot mentioned this pull request Aug 23, 2021

chore(deps-dev): bump rollup from 2.40.0 to 2.56.3 filecoin-project/slate#889

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refine SystemJS export rendering#4199

Refine SystemJS export rendering#4199
lukastaegert merged 2 commits intomasterfrom
improve-systemjs-rendering

lukastaegert commented Aug 2, 2021 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 2, 2021 •

edited

Loading

Uh oh!

codecov Bot commented Aug 2, 2021 •

edited

Loading

Uh oh!

guybedford Aug 2, 2021

Uh oh!

lukastaegert Aug 3, 2021

Uh oh!

guybedford left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

lukastaegert commented Aug 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

github-actions Bot commented Aug 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Thank you for your contribution! ❤️

Uh oh!

codecov Bot commented Aug 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

guybedford Aug 2, 2021

Choose a reason for hiding this comment

Uh oh!

lukastaegert Aug 3, 2021

Choose a reason for hiding this comment

Uh oh!

guybedford left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lukastaegert commented Aug 2, 2021 •

edited

Loading

github-actions Bot commented Aug 2, 2021 •

edited

Loading

codecov Bot commented Aug 2, 2021 •

edited

Loading