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

refactor: do not rely on AST extra properties in plugins #11662

Merged
merged 3 commits into from Jun 4, 2020
Merged

refactor: do not rely on AST extra properties in plugins #11662

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
71972b5
refactor: do not reply on AST extra properties in plugins
JLHwung Jun 2, 2020
2ffc76a
Update packages/babel-plugin-proposal-optional-chaining/src/index.js
JLHwung Jun 3, 2020
99f7986
refactor: address review comments [ci-skip]
JLHwung Jun 4, 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
7 changes: 3 additions & 4 deletions packages/babel-helper-member-expression-to-functions/src/index.js
View file
Open in desktop
Expand Up @@ -167,17 +167,16 @@ const handle = {
const parentIsOptionalCall = parentPath.isOptionalCallExpression({
callee: node,
});
const isParenthesizedMemberCall =
parentPath.isCallExpression({ callee: node }) &&
node.extra?.parenthesized;
// if parentIsCall is true, it implies that node.extra.parenthesized is always true
const parentIsCall = parentPath.isCallExpression({ callee: node });
startingOptional.replaceWith(toNonOptional(startingOptional, baseRef));
if (parentIsOptionalCall) {
if (parent.optional) {
parentPath.replaceWith(this.optionalCall(member, parent.arguments));
} else {
parentPath.replaceWith(this.call(member, parent.arguments));
}
} else if (isParenthesizedMemberCall) {
} else if (parentIsCall) {
// `(a?.#b)()` to `(a == null ? void 0 : a.#b.bind(a))()`
member.replaceWith(this.boundGet(member));
} else {
Expand Down
6 changes: 3 additions & 3 deletions ...s/test/fixtures/private-loose/parenthesized-optional-member-call-with-transform/output.js
View file
Open in desktop
Expand Up @@ -8,7 +8,7 @@ class Foo {
}

test() {
var _o$Foo$self$getSelf, _o$Foo$self$getSelf2, _fn$Foo$self$getSelf, _fn$Foo$self$getSelf2, _o$Foo, _o$Foo$self, _fn, _fn$Foo, _fn$Foo$self, _fn$Foo$self2;
var _o$Foo$self$getSelf, _o$Foo$self$getSelf2, _fn$Foo$self$getSelf, _fn$Foo$self$getSelf2, _o$Foo, _o$Foo$self, _fn, _fn$Foo, _fn$Foo$self;

const o = {
Foo: Foo
Expand All @@ -26,8 +26,8 @@ class Foo {
(o === null || o === void 0 ? void 0 : babelHelpers.classPrivateFieldLooseBase(o.Foo, _m)[_m].bind(o.Foo))().toString();
((_o$Foo$self$getSelf = ((_o$Foo = o.Foo) == null ? void 0 : _o$Foo.self.getSelf.bind(_o$Foo.self))()) === null || _o$Foo$self$getSelf === void 0 ? void 0 : babelHelpers.classPrivateFieldLooseBase(_o$Foo$self$getSelf, _m)[_m].bind(_o$Foo$self$getSelf))();
((_o$Foo$self$getSelf2 = ((_o$Foo$self = o.Foo.self) == null ? void 0 : _o$Foo$self.getSelf.bind(_o$Foo$self))()) === null || _o$Foo$self$getSelf2 === void 0 ? void 0 : babelHelpers.classPrivateFieldLooseBase(_o$Foo$self$getSelf2, _m)[_m].bind(_o$Foo$self$getSelf2))();
((_fn$Foo$self$getSelf = ((_fn = fn()) == null ? void 0 : (_fn$Foo = _fn.Foo) == null ? void 0 : _fn$Foo.self.getSelf.bind(_fn.Foo.self))()) === null || _fn$Foo$self$getSelf === void 0 ? void 0 : babelHelpers.classPrivateFieldLooseBase(_fn$Foo$self$getSelf, _m)[_m].bind(_fn$Foo$self$getSelf))();
((_fn$Foo$self$getSelf2 = (fn == null ? void 0 : (_fn$Foo$self2 = _fn$Foo$self = fn().Foo.self) == null ? void 0 : _fn$Foo$self2.getSelf.bind(_fn$Foo$self))()) === null || _fn$Foo$self$getSelf2 === void 0 ? void 0 : babelHelpers.classPrivateFieldLooseBase(_fn$Foo$self$getSelf2, _m)[_m].bind(_fn$Foo$self$getSelf2))();
((_fn$Foo$self$getSelf = ((_fn = fn()) == null ? void 0 : (_fn$Foo = _fn.Foo) == null ? void 0 : _fn$Foo.self.getSelf.bind(_fn$Foo.self))()) === null || _fn$Foo$self$getSelf === void 0 ? void 0 : babelHelpers.classPrivateFieldLooseBase(_fn$Foo$self$getSelf, _m)[_m].bind(_fn$Foo$self$getSelf))();
((_fn$Foo$self$getSelf2 = (fn == null ? void 0 : (_fn$Foo$self = fn().Foo.self) == null ? void 0 : _fn$Foo$self.getSelf.bind(_fn$Foo$self))()) === null || _fn$Foo$self$getSelf2 === void 0 ? void 0 : babelHelpers.classPrivateFieldLooseBase(_fn$Foo$self$getSelf2, _m)[_m].bind(_fn$Foo$self$getSelf2))();
}

}
Expand Down
4 changes: 2 additions & 2 deletions ...perties/test/fixtures/private/parenthesized-optional-member-call-with-transform/output.js
View file
Open in desktop
Expand Up @@ -4,7 +4,7 @@ class Foo {
}

test() {
var _o$Foo, _o$Foo2, _o$Foo3, _o$Foo$self$getSelf, _o$Foo$self$getSelf2, _fn$Foo$self$getSelf, _fn$Foo$self$getSelf2, _o$Foo4, _o$Foo4$self, _o$Foo$self, _fn, _fn$Foo$self, _fn$Foo, _fn$Foo$self2, _fn$Foo$self3;
var _o$Foo, _o$Foo2, _o$Foo3, _o$Foo$self$getSelf, _o$Foo$self$getSelf2, _fn$Foo$self$getSelf, _fn$Foo$self$getSelf2, _o$Foo4, _o$Foo4$self, _o$Foo$self, _fn, _fn$Foo, _fn$Foo$self, _fn$Foo$self2;

const o = {
Foo: Foo
Expand All @@ -23,7 +23,7 @@ class Foo {
((_o$Foo$self$getSelf = ((_o$Foo4 = o.Foo) === null || _o$Foo4 === void 0 ? void 0 : (_o$Foo4$self = _o$Foo4.self).getSelf.bind(_o$Foo4$self))()) === null || _o$Foo$self$getSelf === void 0 ? void 0 : babelHelpers.classStaticPrivateFieldSpecGet(_o$Foo$self$getSelf, Foo, _m).bind(_o$Foo$self$getSelf))();
((_o$Foo$self$getSelf2 = ((_o$Foo$self = o.Foo.self) === null || _o$Foo$self === void 0 ? void 0 : _o$Foo$self.getSelf.bind(_o$Foo$self))()) === null || _o$Foo$self$getSelf2 === void 0 ? void 0 : babelHelpers.classStaticPrivateFieldSpecGet(_o$Foo$self$getSelf2, Foo, _m).bind(_o$Foo$self$getSelf2))();
((_fn$Foo$self$getSelf = ((_fn = fn()) === null || _fn === void 0 ? void 0 : (_fn$Foo = _fn.Foo) === null || _fn$Foo === void 0 ? void 0 : (_fn$Foo$self = _fn$Foo.self).getSelf.bind(_fn$Foo$self))()) === null || _fn$Foo$self$getSelf === void 0 ? void 0 : babelHelpers.classStaticPrivateFieldSpecGet(_fn$Foo$self$getSelf, Foo, _m).bind(_fn$Foo$self$getSelf))();
((_fn$Foo$self$getSelf2 = (fn === null || fn === void 0 ? void 0 : (_fn$Foo$self3 = _fn$Foo$self2 = fn().Foo.self) === null || _fn$Foo$self3 === void 0 ? void 0 : _fn$Foo$self3.getSelf.bind(_fn$Foo$self2))()) === null || _fn$Foo$self$getSelf2 === void 0 ? void 0 : babelHelpers.classStaticPrivateFieldSpecGet(_fn$Foo$self$getSelf2, Foo, _m).bind(_fn$Foo$self$getSelf2))();
((_fn$Foo$self$getSelf2 = (fn === null || fn === void 0 ? void 0 : (_fn$Foo$self2 = fn().Foo.self) === null || _fn$Foo$self2 === void 0 ? void 0 : _fn$Foo$self2.getSelf.bind(_fn$Foo$self2))()) === null || _fn$Foo$self$getSelf2 === void 0 ? void 0 : babelHelpers.classStaticPrivateFieldSpecGet(_fn$Foo$self$getSelf2, Foo, _m).bind(_fn$Foo$self$getSelf2))();
}

}
Expand Down
23 changes: 14 additions & 9 deletions packages/babel-plugin-proposal-optional-chaining/src/index.js
View file
Open in desktop
Expand Up @@ -24,8 +24,20 @@ export default declare((api, options) => {
visitor: {
"OptionalCallExpression|OptionalMemberExpression"(path) {
const { scope } = path;
const parentPath = path.findParent(p => !p.isParenthesizedExpression());
// maybeParenthesized points to the outermost parenthesizedExpression
// or the path itself
let maybeParenthesized = path;
const parentPath = path.findParent(p => {
if (!p.isParenthesizedExpression()) return true;
maybeParenthesized = p;
});
let isDeleteOperation = false;
const parentIsCall =
parentPath.isCallExpression({ callee: maybeParenthesized.node }) &&
// note that the first condition must implies that `path.optional` is `true`,
// otherwise the parentPath should be an OptionalCallExpressioin
path.isOptionalMemberExpression();

const optionals = [];

let optionalPath = path;
Expand Down Expand Up @@ -116,14 +128,7 @@ export default declare((api, options) => {
}
let replacement = replacementPath.node;
// Ensure (a?.b)() has proper `this`
if (
t.isMemberExpression(replacement) &&
(replacement.extra?.parenthesized ||
// if replacementPath.parentPath does not equal parentPath,
// it must be unwrapped from parenthesized expression.
replacementPath.parentPath !== parentPath) &&
parentPath.isCallExpression()
) {
if (i === 0 && parentIsCall) {
Copy link
Member

Choose a reason for hiding this comment

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

What happens if we remove the i check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question!

Since parentIsCall is computed only once for the baseRef, if we don't limit this branch with i === 0, which means applying to the baseRef only, it will generate multiple memoizing assignment during looping all the optional chain elements.

This change fixes an issue in https://github.com/babel/babel/pull/11662/files#diff-778d6015479bf103795200e754865e9dL30 where fn().Foo.self is memoized twice.

Copy link
Member

Choose a reason for hiding this comment

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

Do we want to add this in a comment?

// `(a?.b)()` to `(a == null ? undefined : a.b.bind(a))()`
const { object } = replacement;
let baseRef;
Expand Down
1 change: 1 addition & 0 deletions packages/babel-plugin-proposal-optional-chaining/test/fixtures/general/containers/input.js
View file
Open in desktop
Expand Up @@ -2,5 +2,6 @@ var street = user.address?.street
street = user.address?.street

test(a?.b, 1);
test((a?.b), 1);

(1, a?.b, 2);
5 changes: 3 additions & 2 deletions packages/babel-plugin-proposal-optional-chaining/test/fixtures/general/containers/output.js
View file
Open in desktop
@@ -1,6 +1,7 @@
var _user$address, _user$address2, _a, _a2;
var _user$address, _user$address2, _a, _a2, _a3;

var street = (_user$address = user.address) === null || _user$address === void 0 ? void 0 : _user$address.street;
street = (_user$address2 = user.address) === null || _user$address2 === void 0 ? void 0 : _user$address2.street;
test((_a = a) === null || _a === void 0 ? void 0 : _a.b, 1);
1, (_a2 = a) === null || _a2 === void 0 ? void 0 : _a2.b, 2;
test((_a2 = a) === null || _a2 === void 0 ? void 0 : _a2.b, 1);
1, (_a3 = a) === null || _a3 === void 0 ? void 0 : _a3.b, 2;
7 changes: 7 additions & 0 deletions ...osal-optional-chaining/test/fixtures/general/parenthesized-expression-containers/input.js
View file
Open in desktop
@@ -0,0 +1,7 @@
var street = user.address?.street
street = user.address?.street

test(a?.b, 1);
test((a?.b), 1);

(1, a?.b, 2);
6 changes: 6 additions & 0 deletions ...-optional-chaining/test/fixtures/general/parenthesized-expression-containers/options.json
View file
Open in desktop
@@ -0,0 +1,6 @@
{
"plugins": ["proposal-optional-chaining"],
"parserOpts": {
"createParenthesizedExpressions": true
}
}
7 changes: 7 additions & 0 deletions ...sal-optional-chaining/test/fixtures/general/parenthesized-expression-containers/output.js
View file
Open in desktop
@@ -0,0 +1,7 @@
var _user$address, _user$address2, _a, _a2, _a3;

var street = (_user$address = user.address) === null || _user$address === void 0 ? void 0 : _user$address.street;
street = (_user$address2 = user.address) === null || _user$address2 === void 0 ? void 0 : _user$address2.street;
test((_a = a) === null || _a === void 0 ? void 0 : _a.b, 1);
test(((_a2 = a) === null || _a2 === void 0 ? void 0 : _a2.b), 1);
((1, (_a3 = a) === null || _a3 === void 0 ? void 0 : _a3.b, 2));
6 changes: 3 additions & 3 deletions ...roposal-optional-chaining/test/fixtures/general/parenthesized-member-call-loose/output.js
View file
Open in desktop
Expand Up @@ -13,7 +13,7 @@ class Foo {
}

test() {
var _o$Foo$self$getSelf, _o$Foo, _o$Foo$self$getSelf2, _o$Foo$self, _fn$Foo$self$getSelf, _fn, _fn$Foo, _fn$Foo$self$getSelf2, _fn$Foo$self, _fn$Foo$self2;
var _o$Foo$self$getSelf, _o$Foo, _o$Foo$self$getSelf2, _o$Foo$self, _fn$Foo$self$getSelf, _fn, _fn$Foo, _fn$Foo$self$getSelf2, _fn$Foo$self;

const Foo = this;
const o = {
Expand All @@ -32,8 +32,8 @@ class Foo {
(o == null ? void 0 : o.Foo.m.bind(o.Foo))().toString();
((_o$Foo$self$getSelf = ((_o$Foo = o.Foo) == null ? void 0 : _o$Foo.self.getSelf.bind(_o$Foo.self))()) == null ? void 0 : _o$Foo$self$getSelf.m.bind(_o$Foo$self$getSelf))();
((_o$Foo$self$getSelf2 = ((_o$Foo$self = o.Foo.self) == null ? void 0 : _o$Foo$self.getSelf.bind(_o$Foo$self))()) == null ? void 0 : _o$Foo$self$getSelf2.m.bind(_o$Foo$self$getSelf2))();
((_fn$Foo$self$getSelf = ((_fn = fn()) == null ? void 0 : (_fn$Foo = _fn.Foo) == null ? void 0 : _fn$Foo.self.getSelf.bind(_fn.Foo.self))()) == null ? void 0 : _fn$Foo$self$getSelf.m.bind(_fn$Foo$self$getSelf))();
((_fn$Foo$self$getSelf2 = (fn == null ? void 0 : (_fn$Foo$self2 = _fn$Foo$self = fn().Foo.self) == null ? void 0 : _fn$Foo$self2.getSelf.bind(_fn$Foo$self))()) == null ? void 0 : _fn$Foo$self$getSelf2.m.bind(_fn$Foo$self$getSelf2))();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

✌️Unnecessary memoizers removed.

((_fn$Foo$self$getSelf = ((_fn = fn()) == null ? void 0 : (_fn$Foo = _fn.Foo) == null ? void 0 : _fn$Foo.self.getSelf.bind(_fn$Foo.self))()) == null ? void 0 : _fn$Foo$self$getSelf.m.bind(_fn$Foo$self$getSelf))();
((_fn$Foo$self$getSelf2 = (fn == null ? void 0 : (_fn$Foo$self = fn().Foo.self) == null ? void 0 : _fn$Foo$self.getSelf.bind(_fn$Foo$self))()) == null ? void 0 : _fn$Foo$self$getSelf2.m.bind(_fn$Foo$self$getSelf2))();
}

}
Expand Down
4 changes: 2 additions & 2 deletions ...ugin-proposal-optional-chaining/test/fixtures/general/parenthesized-member-call/output.js
View file
Open in desktop
Expand Up @@ -13,7 +13,7 @@ class Foo {
}

test() {
var _o$Foo, _o$Foo2, _o$Foo3, _o$Foo$self$getSelf, _o$Foo4, _o$Foo4$self, _o$Foo$self$getSelf2, _o$Foo$self, _fn$Foo$self$getSelf, _fn, _fn$Foo$self, _fn$Foo, _fn$Foo$self$getSelf2, _fn$Foo$self2, _fn$Foo$self3;
var _o$Foo, _o$Foo2, _o$Foo3, _o$Foo$self$getSelf, _o$Foo4, _o$Foo4$self, _o$Foo$self$getSelf2, _o$Foo$self, _fn$Foo$self$getSelf, _fn, _fn$Foo, _fn$Foo$self, _fn$Foo$self$getSelf2, _fn$Foo$self2;

const Foo = this;
const o = {
Expand All @@ -33,7 +33,7 @@ class Foo {
((_o$Foo$self$getSelf = ((_o$Foo4 = o.Foo) === null || _o$Foo4 === void 0 ? void 0 : (_o$Foo4$self = _o$Foo4.self).getSelf.bind(_o$Foo4$self))()) === null || _o$Foo$self$getSelf === void 0 ? void 0 : _o$Foo$self$getSelf.m.bind(_o$Foo$self$getSelf))();
((_o$Foo$self$getSelf2 = ((_o$Foo$self = o.Foo.self) === null || _o$Foo$self === void 0 ? void 0 : _o$Foo$self.getSelf.bind(_o$Foo$self))()) === null || _o$Foo$self$getSelf2 === void 0 ? void 0 : _o$Foo$self$getSelf2.m.bind(_o$Foo$self$getSelf2))();
((_fn$Foo$self$getSelf = ((_fn = fn()) === null || _fn === void 0 ? void 0 : (_fn$Foo = _fn.Foo) === null || _fn$Foo === void 0 ? void 0 : (_fn$Foo$self = _fn$Foo.self).getSelf.bind(_fn$Foo$self))()) === null || _fn$Foo$self$getSelf === void 0 ? void 0 : _fn$Foo$self$getSelf.m.bind(_fn$Foo$self$getSelf))();
((_fn$Foo$self$getSelf2 = (fn === null || fn === void 0 ? void 0 : (_fn$Foo$self3 = _fn$Foo$self2 = fn().Foo.self) === null || _fn$Foo$self3 === void 0 ? void 0 : _fn$Foo$self3.getSelf.bind(_fn$Foo$self2))()) === null || _fn$Foo$self$getSelf2 === void 0 ? void 0 : _fn$Foo$self$getSelf2.m.bind(_fn$Foo$self$getSelf2))();
((_fn$Foo$self$getSelf2 = (fn === null || fn === void 0 ? void 0 : (_fn$Foo$self2 = fn().Foo.self) === null || _fn$Foo$self2 === void 0 ? void 0 : _fn$Foo$self2.getSelf.bind(_fn$Foo$self2))()) === null || _fn$Foo$self$getSelf2 === void 0 ? void 0 : _fn$Foo$self$getSelf2.m.bind(_fn$Foo$self$getSelf2))();
}

}
Expand Down