Extract for-of iterator handling to a helper by nicolo-ribaudo · Pull Request #11262 · babel/babel

nicolo-ribaudo · 2020-03-14T23:26:08Z

Q	A
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?
License	MIT

This greatly reduces the code size (*) when for-of is used multiple times across the
codebase. Also, makes it easier to read what's happening in a compiled loop.

(*) I tested the difference with 3 for loops before adding the human-friendly error message about the missing Symbol.iterator. I didn't use transform-runtime or external-helpers, so these numbers include the helper size.

These are the minified sizes (with terser):

	spec	loose
old	563 bytes	465 bytes
new	457 bytes	349 bytes

Since the error message is a bit more than 100 bytes long, the size improvement probably needs 4 loops rather than one now. However, keep in mind that if I added the human-friendly error message to the old implementation it would have added 100 bytes 3 times, making the size improvement even bigger.

Size test code

loop: for (let x of a) {
  break loop;
}
for (var x of a) {
  a = null;
}
for (x of a) {
  continue;
}

Part 1/n

nicolo-ribaudo · 2020-03-15T12:09:28Z

+// It must be kept for compatibility reasons.
+// TODO (Babel 8): Remove this code.
+
+export default function transformWithoutHelper(loose, path, state) {


This file contains the old plugin code needed when the helper isn't available. I just copy-pasted it without any edit.

nicolo-ribaudo · 2020-03-15T12:13:45Z

+  for (_iteratorHelper.s(); !(_step = _iteratorHelper.n()).done;) {
    var i = _step.value;
    var x;
    var f;


This is a bug unrelated to this PR: f and x should be declared in the _loop function.

I think they have to stay here. If we moved the var inside the _loop function, they would no longer declare the variable in the function scope (it's not a block let/const).

jridgewell · 2020-03-16T01:16:31Z

+
+helpers.createForOfIteratorHelperLoose = helper("7.9.0")`
+  export default function _createForOfIteratorHelperLoose(o) {
+    var i = 0;


Nit: Don't share the variable here.

I mostly did it for code size, since this is a runtime helper, and i could stand both for index and iterator 😅

jridgewell · 2020-03-16T01:21:31Z

+
+        const nodes = builder.build({
+          CREATE_ITERATOR_HELPER: state.addHelper(builder.helper),
+          ITERATOR_HELPER: scope.generateUidIdentifier("iteratorHelper"),


Nit:

Suggested change

ITERATOR_HELPER: scope.generateUidIdentifier("iteratorHelper"),

ITERATOR_HELPER: scope.generateUidIdentifier("iterator"),

jridgewell · 2020-03-16T01:28:14Z

-        // push the rest of the original loop body onto our new body
-        block.body = block.body.concat(node.body.body);
+        t.inherits(container[0], node);
+        t.inherits(container[0].body, node.body);


Does container ever contain more than one element? If not, we can replace the replaceWithMultiple with replaceWith and drop the parent.isLabeledStatement() check.

container always contains a single node. However:

We are doing replaceWithMultiple(nodes), and in non-loose mode nodes contains 2 nodes (the var declaration and the try statement)

We can't drop the parent.isLabeledStatement() check because in non-loose mode it the label must be moved to the inner loop. If we only did path.replaceWith() it would be attached to the try statement.

If we only did path.replaceWith() it would be attached to the try statement.

I think that would actually be equivalent. The finally block should still fire correctly.

But then, it doesn't work with continue.

Ohhh. I was only thinking about break.

JLHwung · 2020-03-16T18:33:16Z

+        );
+
+        if (
+          t.isIdentifier(left) ||


nit: It suffices to check t.isVariableDeclaration first and assuming it's LVal in the else branch, because we have validated ForOfStatement.left even when BABEL_TYPES_8_BREAKING is not enabled:

babel/packages/babel-types/src/definitions/es2015.js

Line 333 in 1ba41f2

return assertNodeType("VariableDeclaration", "LVal");

(Oh it is copied from the old codes.)

Dis greatly recudes the code size when for-of is used multiple times across the codebase. Also, makes it easier to read what's happening in a compiled loop.

nicolo-ribaudo added the PR: Internal 🏠 A type of pull request used for our changelog categories label Mar 14, 2020

nicolo-ribaudo force-pushed the iterables-dx/for-of-helper branch from 2c08b9b to 40ce663 Compare March 14, 2020 23:34

nicolo-ribaudo mentioned this pull request Mar 14, 2020

Add for-of fallback for arrays in browsers without symbol support #11263

Merged

1 task

nicolo-ribaudo commented Mar 15, 2020

View reviewed changes

nicolo-ribaudo force-pushed the iterables-dx/for-of-helper branch from d556229 to f505349 Compare March 15, 2020 12:10

nicolo-ribaudo commented Mar 15, 2020

View reviewed changes

nicolo-ribaudo marked this pull request as ready for review March 15, 2020 13:58

jridgewell approved these changes Mar 16, 2020

View reviewed changes

JLHwung reviewed Mar 16, 2020

View reviewed changes

JLHwung approved these changes Mar 16, 2020

View reviewed changes

nicolo-ribaudo added 5 commits March 17, 2020 00:40

Extract for-of iterator handling to a helper

2f4220a

Dis greatly recudes the code size when for-of is used multiple times across the codebase. Also, makes it easier to read what's happening in a compiled loop.

Unify spec and loose code

986a3d7

Legacy implementation fallback

054cf39

Update tmp var name

8aadff9

Updates from review and after rebase

771986f

nicolo-ribaudo force-pushed the iterables-dx/for-of-helper branch from 4173b6c to 771986f Compare March 16, 2020 23:45

nicolo-ribaudo merged commit 5997668 into master Mar 16, 2020

This was referenced Apr 11, 2020

[Snyk] Upgrade @babel/runtime from 7.8.7 to 7.9.0 wreiske/asteriskmonitor#16

Merged

[Snyk] Upgrade: @babel/preset-env, @babel/runtime bee22193/mnmlurl#22

Open

[Snyk] Upgrade @babel/runtime from 7.4.4 to 7.9.0 mboocannon/msgraph-sdk-javascript#9

Open

github-actions Bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jun 16, 2020

github-actions Bot locked as resolved and limited conversation to collaborators Jun 16, 2020

nicolo-ribaudo deleted the iterables-dx/for-of-helper branch June 18, 2020 23:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Extract for-of iterator handling to a helper#11262

Extract for-of iterator handling to a helper#11262
nicolo-ribaudo merged 5 commits into
masterfrom
iterables-dx/for-of-helper

nicolo-ribaudo commented Mar 14, 2020 •

edited

Loading

Uh oh!

nicolo-ribaudo Mar 15, 2020

Uh oh!

nicolo-ribaudo Mar 15, 2020

Uh oh!

jridgewell Mar 16, 2020

Uh oh!

jridgewell Mar 16, 2020

Uh oh!

nicolo-ribaudo Mar 16, 2020

Uh oh!

jridgewell Mar 16, 2020

Uh oh!

jridgewell Mar 16, 2020

Uh oh!

nicolo-ribaudo Mar 16, 2020

Uh oh!

jridgewell Mar 16, 2020

Uh oh!

nicolo-ribaudo Mar 16, 2020

Uh oh!

jridgewell Mar 16, 2020

Uh oh!

JLHwung Mar 16, 2020 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	ITERATOR_HELPER: scope.generateUidIdentifier("iteratorHelper"),
	ITERATOR_HELPER: scope.generateUidIdentifier("iterator"),

Uh oh!

Conversation

nicolo-ribaudo commented Mar 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

JLHwung Mar 16, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nicolo-ribaudo commented Mar 14, 2020 •

edited

Loading

JLHwung Mar 16, 2020 •

edited

Loading