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.

Sign up for GitHub

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

Jump to bottom

fix:added check for forXstatement pattern #11703

Merged
merged 6 commits into from Jun 12, 2020
Merged

fix:added check for forXstatement pattern #11703

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d4c0a5e
added check for forXstatement pattern
wlawt Jun 10, 2020
71f4c24
test case:check node left of for-of
wlawt Jun 10, 2020
79f5619
add node check from feedback
wlawt Jun 11, 2020
a45c4dc
add additional test cases & comment for-of & for-in
wlawt Jun 11, 2020
fff451b
Update packages/babel-helper-member-expression-to-functions/src/index.js
wlawt Jun 11, 2020
d07e916
Update packages/babel-helper-member-expression-to-functions/src/index.js
wlawt Jun 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/babel-helper-member-expression-to-functions/src/index.js
View file
Open in desktop
Expand Up @@ -337,6 +337,8 @@ const handle = {
// [MEMBER = _VALUE] = ARR -> [_destructureSet(MEMBER) = _VALUE] = ARR
// [...MEMBER] -> [..._destructureSet(MEMBER)]
if (
existentialism marked this conversation as resolved.
Show resolved Hide resolved
(parentPath.isForXStatement() &&
parentPath.parentPath.isAssignmentPattern({ left: node })) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

If a path is a ForXStatement, how can its parentPath be an AssignmentPattern? Note that the children of an Pattern is always an expression. A statement can not be an expression.

Here is an AST example of for (o.p of arr);

// for (o.p of arr);
{
  "type": "ForOfStatement",
  "await": false,
  "left": {
    "type": "MemberExpression",
    "object": { "type": "Identifier", "name": "o" },
    "property": { "type": "Identifier", "name": "p" }
   },
  "right": { "type": "Identifier", "name": "arr" }
}

The parentPath here refers to the NodePath of ForOfStatement. NodePath is a wrapper of the AST node that provides parent association with other NodePath. In this PR We would like to make sure that member, which refers to MemberExpression here, is indeed the left of the underlying AST node of parentPath.

So we can use

parentPath.isForXStatement() && parentPath.node.left === node

The query function isForXStatement accepts an additional parameter as query, so the code above can be simplified as

parentPath.isForXStatement({ left: node })

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, when I run the test I seem to get the same results, which shouldn't happen?

input

class D {
  #arr;
  f() {
    for (const el of this.#arr);
  }
}

output

var _arr = new WeakMap();

var D = /*#__PURE__*/function () {
  "use strict";

  function D() {
    babelHelpers.classCallCheck(this, D);

    _arr.set(this, {
      writable: true,
      value: void 0
    });
  }

  babelHelpers.createClass(D, [{
    key: "f",
    value: function f() {
      for (var el of babelHelpers.classPrivateFieldGet(this, _arr)) {
        ;
      }
    }
  }]);
  return D;
}();

Copy link
Contributor

Choose a reason for hiding this comment

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

The output is expected. Can you add a test case from the original issue?

class D {
  #arr;
  f() {
    for (const this.#arr of [1, 2]);
  }
}

// { KEY: MEMBER } = OBJ
(parentPath.isObjectProperty({ value: node }) &&
parentPath.parentPath.isObjectPattern()) ||
Expand Down
6 changes: 6 additions & 0 deletions ...proposal-class-properties/test/fixtures/private/1-helpermemberexpressionfunction/input.js
View file
Open in desktop
@@ -0,0 +1,6 @@
class D {
#arr;
f() {
for (const el of this.#arr);
Copy link
Member

Choose a reason for hiding this comment

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

These should be reversed.

Please also add a test for for-in.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is a regression check on the first draft implementation: https://github.com/babel/babel/pull/11703/files/d4c0a5e8b75306f918cf2a2307e4f65130f9ea05#diff-decaf745088978f783d453709a8a8479

Copy link
Member

Choose a reason for hiding this comment

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

Ooo, ok. So we need 2 new test cases (for-of and for-in) with it appearing in the left.

Copy link
Member

Choose a reason for hiding this comment

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

👍 can just do it in the next line, or leave a comment that it shouldn't be transformed

Copy link
Member

Choose a reason for hiding this comment

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

personally I think just doing them all in the same class is fine

for (const el of this.#arr);
for (this.#arr of [1, 2]);
for (this.#arr in [1,2,3]);

the empty arr seems to be the same case?

and I would rename 1-helpermemberexpressionfunction to something like for-of-member-expression or something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for sure, will remember for next time!

}
}
24 changes: 24 additions & 0 deletions ...roposal-class-properties/test/fixtures/private/1-helpermemberexpressionfunction/output.js
View file
Open in desktop
@@ -0,0 +1,24 @@
var _arr = new WeakMap();

var D = /*#__PURE__*/function () {
"use strict";

function D() {
babelHelpers.classCallCheck(this, D);

_arr.set(this, {
writable: true,
value: void 0
});
}

babelHelpers.createClass(D, [{
key: "f",
value: function f() {
for (var el of babelHelpers.classPrivateFieldGet(this, _arr)) {
;
}
}
}]);
return D;
}();