Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refine SystemJS export rendering #4199

Merged
merged 2 commits into from Aug 5, 2021
Merged

Conversation

lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Aug 2, 2021

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

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

github-actions bot commented Aug 2, 2021

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

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

codecov bot commented Aug 2, 2021

Codecov Report

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

Impacted file tree graph

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


var k, l;
var k, l; exports({ k: k, l: l });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@lukastaegert lukastaegert merged commit b34a411 into master Aug 5, 2021
@lukastaegert lukastaegert deleted the improve-systemjs-rendering branch August 5, 2021 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants