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

Not depending on return value of super(). Fixes #9020. #9060

Merged
merged 4 commits into from Dec 4, 2018
Merged

Not depending on return value of super(). Fixes #9020. #9060

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f3a3925
Not depending on return value of super(). Fixes #9020.
joeldenning Nov 22, 2018
f3ecd00
Merge branch 'master' of github.com:babel/babel into issue-9020
joeldenning Dec 3, 2018
7d646b0
Feedback from nicolo-ribaudo
joeldenning Dec 3, 2018
5a223ac
Feedback -- fixing bad call to replaceWithMultiple
joeldenning Dec 4, 2018
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: 5 additions & 2 deletions packages/babel-plugin-transform-arrow-functions/test/fixtures/arrow-functions/this/output.js
View file
Open in desktop
Expand Up @@ -15,9 +15,12 @@ class Foo extends function () {} {
};

if (true) {
console.log(_this2 = super(), foo());
var _temp;

console.log((_temp = super(), _this2 = this, _temp), foo());
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't fully fix the issue: in edge it would log undefined, foo() instead of this, foo().
You need to use replaceWithMultiple("super call", "this assignment"), because insertAfter keep the initial completion value (undefined in edge).

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 catch - @nicolo-ribaudo I just updated it to do what you said. Let me know if I misunderstood.

} else {
_this2 = super();
super();
_this2 = this;
console.log(foo());
}
}
Expand Down
8 changes: 6 additions & 2 deletions packages/babel-traverse/src/path/conversion.js
View file
Open in desktop
Expand Up @@ -461,8 +461,12 @@ function getThisBinding(thisEnvFn, inConstructor) {
if (supers.has(child.node)) return;
supers.add(child.node);

child.replaceWith(
t.assignmentExpression("=", t.identifier(thisBinding), child.node),
child.insertAfter(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See this comment for why I made this change

t.assignmentExpression(
"=",
t.identifier(thisBinding),
t.identifier("this"),
),
);
},
});
Expand Down
24 changes: 19 additions & 5 deletions packages/babel-traverse/test/arrow-transform.js
View file
Open in desktop
Expand Up @@ -82,7 +82,11 @@ describe("arrow function conversion", () => {
() => this;
`,
`
var _supercall = (..._args) => _this = super(..._args),
var _supercall = (..._args) => {
var _temp;

return _temp = super(..._args), _this = this, _temp;
},
_this;

(function () {
Expand Down Expand Up @@ -115,9 +119,14 @@ describe("arrow function conversion", () => {
(function () {
_this;
});
_this = super();
super();
_this = this;
this;
() => _this = super();
() => {
var _temp;

return _temp = super(), _this = this, _temp;
}
() => this;
`,
{ methodName: "constructor", extend: true },
Expand All @@ -144,9 +153,14 @@ describe("arrow function conversion", () => {

_this;
}).bind(_arrowCheckId);
_this = super();
super();
_this = this;
this;
() => _this = super();
() => {
var _temp;

return _temp = super(), _this = this, _temp;
}
() => this;
`,
{
Expand Down