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: ensure (a?.b)() has proper this #11623

Merged
merged 12 commits into from Jun 1, 2020
Merged

fix: ensure (a?.b)() has proper this #11623

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e8b4108
fix: ensure (a?.b)() has proper this
JLHwung May 12, 2020
a351a4b
let test be more restrictive
JLHwung May 12, 2020
a487e9b
fix: transformed member call should preserve computed
JLHwung May 27, 2020
4f3369f
chore: revamp test files
JLHwung May 27, 2020
3672f7f
refactor: simplify
JLHwung May 27, 2020
30b4852
fix: unwrap parthenthesizedExpression
JLHwung May 27, 2020
792b5bd
add loose test cases
JLHwung May 27, 2020
bd78255
add `(a?.#b)()` support
JLHwung May 28, 2020
7691248
add with-transform test cases
JLHwung May 28, 2020
0c6759b
Update packages/babel-plugin-proposal-optional-chaining/src/index.js
JLHwung May 30, 2020
44e02e9
address review comments
JLHwung May 30, 2020
a2d5657
update test fixtures
JLHwung May 30, 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
14 changes: 14 additions & 0 deletions packages/babel-helper-create-class-features-plugin/src/fields.js
View file
Open in desktop
Expand Up @@ -241,6 +241,13 @@ const privateNameHandlerSpec = {
]);
},

boundGet(member) {
return t.callExpression(
t.memberExpression(this.get(member), t.identifier("bind")),
JLHwung marked this conversation as resolved.
Show resolved Hide resolved
[this.receiver(member)],
);
},

set(member, value) {
const { classRef, privateNamesMap, file } = this;
const { name } = member.node.property.id;
Expand Down Expand Up @@ -323,6 +330,13 @@ const privateNameHandlerLoose = {
});
},

boundGet(member) {
return t.callExpression(
t.memberExpression(this.get(member), t.identifier("bind")),
[member.node.object],
JLHwung marked this conversation as resolved.
Show resolved Hide resolved
);
},

simpleSet(member) {
return this.get(member);
},
Expand Down
6 changes: 6 additions & 0 deletions packages/babel-helper-member-expression-to-functions/src/index.js
View file
Open in desktop
Expand Up @@ -167,13 +167,19 @@ const handle = {
const parentIsOptionalCall = parentPath.isOptionalCallExpression({
callee: node,
});
const isParenthesizedMemberCall =
parentPath.isCallExpression({ callee: node }) &&
node.extra?.parenthesized;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current approach does not support parserOpts.createParenthesizedExpressions. When developing I realize that the whole transform does not take ParenthesizedExpressions into account, I suggest we address it in a separate PR before this one gets too big to review.

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) {
// `(a?.#b)()` to `(a == null ? void 0 : a.#b.bind(a))()`
member.replaceWith(this.boundGet(member));
} else {
member.replaceWith(this.get(member));
}
Expand Down
36 changes: 36 additions & 0 deletions ...l-class-properties/test/fixtures/private-loose/parenthesized-optional-member-call/exec.js
View file
Open in desktop
@@ -0,0 +1,36 @@
class Foo {
static #x = 1;

static self = Foo;
static #m = function() { return this.#x; };
static getSelf() { return Foo }

test() {
const o = { Foo: Foo };

expect((Foo?.#m)()).toEqual(1);
expect((Foo?.#m)().toString).toEqual(1..toString);
expect((Foo?.#m)().toString()).toEqual('1');

expect((o?.Foo.#m)()).toEqual(1);
expect((o?.Foo.#m)().toString).toEqual(1..toString);
expect((o?.Foo.#m)().toString()).toEqual('1');

expect((((o.Foo?.self.getSelf)())?.#m)()).toEqual(1);
expect((((o.Foo.self?.getSelf)())?.#m)()).toEqual(1);
}

testNull() {
const o = null;

expect(() => { (o?.Foo.#m)() }).toThrow();
expect(() => { (o?.Foo.#m)().toString }).toThrow();
expect(() => { (o?.Foo.#m)().toString() }).toThrow();

expect(() => { (((o.Foo?.self.getSelf)())?.#m)() }).toThrow();
expect(() => { (((o.Foo.self?.getSelf)())?.#m)() }).toThrow();
}
}

(new Foo).test();
(new Foo).testNull();
24 changes: 24 additions & 0 deletions ...-class-properties/test/fixtures/private-loose/parenthesized-optional-member-call/input.js
View file
Open in desktop
@@ -0,0 +1,24 @@
class Foo {
static #x = 1;

static self = Foo;
static #m = function() { return this.#x; };
static getSelf() { return Foo }

test() {
const o = { Foo: Foo };

(Foo?.#m)();
(Foo?.#m)().toString;
(Foo?.#m)().toString();

(o?.Foo.#m)();
(o?.Foo.#m)().toString;
(o?.Foo.#m)().toString();

(((o.Foo?.self.getSelf)())?.#m)();
(((o.Foo.self?.getSelf)())?.#m)();
}
}

(new Foo).test();
8 changes: 8 additions & 0 deletions ...ss-properties/test/fixtures/private-loose/parenthesized-optional-member-call/options.json
View file
Open in desktop
@@ -0,0 +1,8 @@
{
"plugins": [
["external-helpers", { "helperVersion": "7.100.0" }],
["proposal-class-properties", { "loose": true }],
"transform-classes",
"transform-block-scoping"
]
}
49 changes: 49 additions & 0 deletions ...class-properties/test/fixtures/private-loose/parenthesized-optional-member-call/output.js
View file
Open in desktop
@@ -0,0 +1,49 @@
var Foo = /*#__PURE__*/function () {
"use strict";

function Foo() {
babelHelpers.classCallCheck(this, Foo);
}

babelHelpers.createClass(Foo, [{
key: "test",
value: function test() {
var _o$Foo$self$getSelf, _o$Foo$self$getSelf2;

var o = {
Foo: Foo
};
(Foo === null || Foo === void 0 ? void 0 : babelHelpers.classPrivateFieldLooseBase(Foo, _m)[_m].bind(Foo))();
(Foo === null || Foo === void 0 ? void 0 : babelHelpers.classPrivateFieldLooseBase(Foo, _m)[_m].bind(Foo))().toString;
(Foo === null || Foo === void 0 ? void 0 : babelHelpers.classPrivateFieldLooseBase(Foo, _m)[_m].bind(Foo))().toString();
(o === null || o === void 0 ? void 0 : babelHelpers.classPrivateFieldLooseBase(o.Foo, _m)[_m].bind(o.Foo))();
(o === null || o === void 0 ? void 0 : babelHelpers.classPrivateFieldLooseBase(o.Foo, _m)[_m].bind(o.Foo))().toString;
(o === null || o === void 0 ? void 0 : babelHelpers.classPrivateFieldLooseBase(o.Foo, _m)[_m].bind(o.Foo))().toString();
((_o$Foo$self$getSelf = (o.Foo?.self.getSelf)()) === 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?.getSelf)()) === null || _o$Foo$self$getSelf2 === void 0 ? void 0 : babelHelpers.classPrivateFieldLooseBase(_o$Foo$self$getSelf2, _m)[_m].bind(_o$Foo$self$getSelf2))();
}
}], [{
key: "getSelf",
value: function getSelf() {
return Foo;
}
}]);
return Foo;
}();

var _x = babelHelpers.classPrivateFieldLooseKey("x");

var _m = babelHelpers.classPrivateFieldLooseKey("m");

Object.defineProperty(Foo, _x, {
writable: true,
value: 1
});
Foo.self = Foo;
Object.defineProperty(Foo, _m, {
writable: true,
value: function () {
return babelHelpers.classPrivateFieldLooseBase(this, _x)[_x];
}
});
new Foo().test();
36 changes: 36 additions & 0 deletions ...roposal-class-properties/test/fixtures/private/parenthesized-optional-member-call/exec.js
View file
Open in desktop
@@ -0,0 +1,36 @@
class Foo {
static #x = 1;

static self = Foo;
static #m = function() { return this.#x; };
static getSelf() { return Foo }

test() {
const o = { Foo: Foo };

expect((Foo?.#m)()).toEqual(1);
expect((Foo?.#m)().toString).toEqual(1..toString);
expect((Foo?.#m)().toString()).toEqual('1');

expect((o?.Foo.#m)()).toEqual(1);
expect((o?.Foo.#m)().toString).toEqual(1..toString);
expect((o?.Foo.#m)().toString()).toEqual('1');

expect((((o.Foo?.self.getSelf)())?.#m)()).toEqual(1);
expect((((o.Foo.self?.getSelf)())?.#m)()).toEqual(1);
}

testNull() {
const o = null;

expect(() => { (o?.Foo.#m)() }).toThrow();
expect(() => { (o?.Foo.#m)().toString }).toThrow();
expect(() => { (o?.Foo.#m)().toString() }).toThrow();

expect(() => { (((o.Foo?.self.getSelf)())?.#m)() }).toThrow();
expect(() => { (((o.Foo.self?.getSelf)())?.#m)() }).toThrow();
}
}

(new Foo).test();
(new Foo).testNull();
24 changes: 24 additions & 0 deletions ...oposal-class-properties/test/fixtures/private/parenthesized-optional-member-call/input.js
View file
Open in desktop
@@ -0,0 +1,24 @@
class Foo {
static #x = 1;

static self = Foo;
static #m = function() { return this.#x; };
static getSelf() { return Foo }

test() {
const o = { Foo: Foo };

(Foo?.#m)();
(Foo?.#m)().toString;
(Foo?.#m)().toString();

(o?.Foo.#m)();
(o?.Foo.#m)().toString;
(o?.Foo.#m)().toString();

(((o.Foo?.self.getSelf)())?.#m)();
(((o.Foo.self?.getSelf)())?.#m)();
}
}

(new Foo).test();
8 changes: 8 additions & 0 deletions ...al-class-properties/test/fixtures/private/parenthesized-optional-member-call/options.json
View file
Open in desktop
@@ -0,0 +1,8 @@
{
"plugins": [
["external-helpers", { "helperVersion": "7.100.0" }],
"proposal-class-properties",
"transform-classes",
"transform-block-scoping"
]
}
45 changes: 45 additions & 0 deletions ...posal-class-properties/test/fixtures/private/parenthesized-optional-member-call/output.js
View file
Open in desktop
@@ -0,0 +1,45 @@
var Foo = /*#__PURE__*/function () {
"use strict";

function Foo() {
babelHelpers.classCallCheck(this, Foo);
}

babelHelpers.createClass(Foo, [{
key: "test",
value: function test() {
var _o$Foo$self$getSelf, _o$Foo$self$getSelf2;

var o = {
Foo: Foo
};
(Foo === null || Foo === void 0 ? void 0 : babelHelpers.classStaticPrivateFieldSpecGet(Foo, Foo, _m).bind(Foo))();
(Foo === null || Foo === void 0 ? void 0 : babelHelpers.classStaticPrivateFieldSpecGet(Foo, Foo, _m).bind(Foo))().toString;
(Foo === null || Foo === void 0 ? void 0 : babelHelpers.classStaticPrivateFieldSpecGet(Foo, Foo, _m).bind(Foo))().toString();
(o === null || o === void 0 ? void 0 : babelHelpers.classStaticPrivateFieldSpecGet(o.Foo, Foo, _m).bind(o.Foo))();
(o === null || o === void 0 ? void 0 : babelHelpers.classStaticPrivateFieldSpecGet(o.Foo, Foo, _m).bind(o.Foo))().toString;
(o === null || o === void 0 ? void 0 : babelHelpers.classStaticPrivateFieldSpecGet(o.Foo, Foo, _m).bind(o.Foo))().toString();
((_o$Foo$self$getSelf = (o.Foo?.self.getSelf)()) === 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?.getSelf)()) === null || _o$Foo$self$getSelf2 === void 0 ? void 0 : babelHelpers.classStaticPrivateFieldSpecGet(_o$Foo$self$getSelf2, Foo, _m).bind(_o$Foo$self$getSelf2))();
}
}], [{
key: "getSelf",
value: function getSelf() {
return Foo;
}
}]);
return Foo;
}();

var _x = {
writable: true,
value: 1
};
babelHelpers.defineProperty(Foo, "self", Foo);
var _m = {
writable: true,
value: function () {
return babelHelpers.classStaticPrivateFieldSpecGet(this, Foo, _x);
}
};
new Foo().test();
52 changes: 44 additions & 8 deletions packages/babel-plugin-proposal-optional-chaining/src/index.js
View file
Open in desktop
Expand Up @@ -23,14 +23,23 @@ export default declare((api, options) => {

visitor: {
"OptionalCallExpression|OptionalMemberExpression"(path) {
const { parentPath, scope } = path;
const { scope } = path;
let parentPath;
// unwrap parenthesis around parent path
for (
{ parentPath } = path;
parentPath.type === "ParenthesizedExpression";
{ parentPath } = parentPath
);
JLHwung marked this conversation as resolved.
Show resolved Hide resolved
let isDeleteOperation = false;
const optionals = [];

let optionalPath = path;
while (
optionalPath.isOptionalMemberExpression() ||
optionalPath.isOptionalCallExpression()
optionalPath.isOptionalCallExpression() ||
optionalPath.isParenthesizedExpression() ||
optionalPath.isTSNonNullExpression()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By doing so we also fix cases like a!!?.b are not transformed currently.

Copy link
Member

Choose a reason for hiding this comment

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

@oliverdunk Should we also check for this in your PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, I'll add that now 👍

) {
const { node } = optionalPath;
if (node.optional) {
Expand All @@ -43,10 +52,8 @@ export default declare((api, options) => {
} else if (optionalPath.isOptionalCallExpression()) {
optionalPath.node.type = "CallExpression";
optionalPath = optionalPath.get("callee");
}

// unwrap a TSNonNullExpression if need
if (optionalPath.isTSNonNullExpression()) {
} else {
// unwrap TSNonNullExpression/ParenthesizedExpression if needed
optionalPath = optionalPath.get("expression");
}
}
Expand Down Expand Up @@ -113,7 +120,36 @@ 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()
) {
// `(a?.b)()` to `(a == null ? undefined : a.b.bind(a))()`
const { object } = replacement;
let baseRef;
if (!loose) {
// memoize the context object in non-loose mode
// `(a?.b.c)()` to `(a == null ? undefined : (_a$b = a.b).c.bind(_a$b))()`
JLHwung marked this conversation as resolved.
Show resolved Hide resolved
baseRef = scope.maybeGenerateMemoised(object);
if (baseRef) {
replacement.object = t.assignmentExpression(
"=",
baseRef,
object,
);
}
}
replacement = t.callExpression(
t.memberExpression(replacement, t.identifier("bind")),
[t.cloneNode(baseRef ?? object)],
);
}
replacementPath.replaceWith(
t.conditionalExpression(
loose
Expand All @@ -134,7 +170,7 @@ export default declare((api, options) => {
isDeleteOperation
? t.booleanLiteral(true)
: scope.buildUndefinedNode(),
replacementPath.node,
replacement,
),
);

Expand Down