Skip to content

Switch to arrow for ts class wrapper IIFE #18027

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

Merged
merged 2 commits into from
Aug 25, 2017
Merged

Switch to arrow for ts class wrapper IIFE #18027

merged 2 commits into from
Aug 25, 2017

Conversation

rbuckton
Copy link
Member

This change switches the TypeScript class wrapper IIFE created by the ts transformer to use an arrow function rather than a function expression. This allows the es2015 transform to accurately determine whether it should capture this as _this in the correct places.

Fixes #16924

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

I can't tell if there are relevant tests with this change. The code itself looks good.

@@ -124,6 +124,7 @@ var stringOrNumberOrUndefined = C.inStaticNestedArrowFunction;


//// [output.js]
var _this = this;
Copy link
Member

Choose a reason for hiding this comment

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

which tests were incorrect before this change? This is the closest one that I've found.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Approved after the test from the bug is added.

@rbuckton rbuckton merged commit 05402b8 into master Aug 25, 2017
@rbuckton
Copy link
Member Author

This has been ported to release-2.5

@rbuckton rbuckton deleted the fix16924 branch August 25, 2017 00:03
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants