Not depending on return value of super(). Fixes #9020. #9060
Conversation
| @@ -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( | |||
There was a problem hiding this comment.
See this comment for why I made this change
|
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()); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
👍 good catch - @nicolo-ribaudo I just updated it to do what you said. Let me know if I misunderstood.
danez
added
PR: Bug Fix 🐛
joeldenning
force-pushed
the
issue-9020
branch
from
fb57c17
to
7d646b0
Compare
| @@ -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( | |||
There was a problem hiding this comment.
@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.
|
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? |
|
I think the reason that there aren't more tests affected is that, if i understand the code correctly, this only affects |
|
Is there a risk this fix break #2279 ? |
|
It shouldn't; this PR doesn't touch transpilation of |
|
Hey, any chance this could be released? Thanks 🙂 |
lock
bot
added
the
outdated
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.