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.

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

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

Merged
merged 4 commits into from Dec 4, 2018

Conversation

@joeldenning
Copy link
Contributor

joeldenning commented Nov 22, 2018

Q                       A
Fixed Issues? Fixes #9020
Patch: Bug Fix? 👍
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? 👍
Documentation PR Link N/A
Any Dependency Changes? No
License MIT

See #9020 and this comment where @nicolo-ribaudo suggested the change that I implemented here. This is my first contribution to babel and I'm not very familiar with the codebase -- I welcome any feedback. I consider this to be a best effort implementation, since I probably am missing some things or don't fully understand the implications of my code changes.

@@ -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(

This comment has been minimized.

Copy link
@joeldenning

joeldenning Nov 22, 2018

Author Contributor

See this comment for why I made this change

@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Nov 22, 2018

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/9541/

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

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

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Nov 23, 2018

Member

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).

This comment has been minimized.

Copy link
@joeldenning

joeldenning Dec 3, 2018

Author Contributor

👍 good catch - @nicolo-ribaudo I just updated it to do what you said. Let me know if I misunderstood.

@joeldenning joeldenning force-pushed the joeldenning:issue-9020 branch from fb57c17 to 7d646b0 Dec 3, 2018
Copy link
Member

nicolo-ribaudo left a comment

I would have expected many more tests to change 🤔

@@ -461,8 +461,13 @@ function getThisBinding(thisEnvFn, inConstructor) {
if (supers.has(child.node)) return;
supers.add(child.node);

child.replaceWith(
t.assignmentExpression("=", t.identifier(thisBinding), child.node),
child.replaceWithMultiple(

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Dec 4, 2018

Member

It takes an array.

This comment has been minimized.

Copy link
@joeldenning

joeldenning Dec 4, 2018

Author Contributor

@nicolo-ribaudo oops! I just fixed that. I don't fully understand the expected output in some of the tests, so I didn't catch that the second argument was being ignored.

Copy link
Member

nicolo-ribaudo left a comment

I'm surpiprised that only two tests changed, but LGTM 👍

@joeldenning

This comment has been minimized.

Copy link
Contributor Author

joeldenning commented Dec 4, 2018

Yeah I was surprised that there weren't more tests affected, too. Are you the one who will merge this @nicolo-ribaudo? Or is that someone else?

@joeldenning

This comment has been minimized.

Copy link
Contributor Author

joeldenning commented Dec 4, 2018

I think the reason that there aren't more tests affected is that, if i understand the code correctly, this only affects super() calls in classes that have arrow functions in the constructor (instead of just all of the super calls everywhere). I think the most common case for that is compiled class properties that use the fn1 = () => {} syntax, because those end up being arrow functions in the constructor. I'm not 100% sure on that, but that's what I saw in my own testing of it.

@nicolo-ribaudo nicolo-ribaudo merged commit d305419 into babel:master Dec 4, 2018
4 checks passed
4 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 80.7% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@JSteunou

This comment has been minimized.

Copy link

JSteunou commented Dec 4, 2018

Is there a risk this fix break #2279 ?

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Dec 4, 2018

It shouldn't; this PR doesn't touch transpilation of super().

@joeldenning joeldenning deleted the joeldenning:issue-9020 branch Dec 5, 2018
@adrienharnay

This comment has been minimized.

Copy link

adrienharnay commented Dec 10, 2018

Hey, any chance this could be released? Thanks 🙂

@lock lock bot added the outdated label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants
You can’t perform that action at this time.