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

Keep class properties in ESNext target #10

Merged

Conversation

joeywatts
Copy link

@joeywatts joeywatts commented Sep 21, 2018

Moves the transformation of class properties to the ESNext transformer.

Property initializers are moved to the constructor by the TypeScript transformer only if there is constructor parameter properties. This is to preserve property initialization order.

It currently does not produce void 0 initializers for property declarations that have no initializer due to ESNext and TypeScript having different runtime behaviors. TS completely elides property declarations with no initializer, while I believe the ESNext would define that property on the instance with an undefined value (which is observable at runtime - propertyName in instance or for (var prop in instance) {}).

@mheiber

This comment has been minimized.

let transformFlags = subtreeFlags | TransformFlags.AssertESNext;

// Decorators, TypeScript-specific modifiers, and type annotations are TypeScript syntax.
if (node.decorators
Copy link

Choose a reason for hiding this comment

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

Do we want to check node.decorators.length instead of node.decorators?

return isInitializedProperty(member, /*isStatic*/ false);
}

/**
Copy link

Choose a reason for hiding this comment

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

I recommend deleting this doc comment: it's longer than the function and doesn't add any information beyond what the signature says.

@@ -3253,7 +3253,6 @@ namespace ts {
|| hasModifier(node, ModifierFlags.TypeScriptModifier)
Copy link

@mheiber mheiber Sep 27, 2018

Choose a reason for hiding this comment

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

nice work.
It seems to me that the PR does two thing:

  1. move code from the typescript transformer to the ESNext transformer
  2. only move property initializers to the constructor when there are constructor parameter properties.

Is there anything else? Just checking to make sure I follow

Copy link

@mheiber mheiber Oct 4, 2018

Choose a reason for hiding this comment

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

I also understand that you did some work on the interaction of decorators and computed properties. But I can't remember why we needed new code to handle it (I know there was a reason, I just didn't take good enough notes)

Copy link
Author

Choose a reason for hiding this comment

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

The transformation of TypeScript's experimental decorators was sort of entangled in the property transformation that was taking place in the TypeScript transformation. Recall that the goal is to move the class field transformation to the ESNext transformer, but we want to keep the transformation of experimental decorators in the TypeScript transformer.

When transforming decorated properties with computed names, the TypeScript transformer must create a hoisted variable to store the computed property name to avoid it being evaluated twice (once in the initialization of the property and again in the call to the decorator function). Before, the class field transformation and decorator transformation were happening all at once and the declarations were being omitted, but now, we'd like to preserve the class field declarations in the TypeScript transform, and leave the logic of transforming them to the ESNext transform. To accomplish this, the TypeScript transformer only creates a hoisted property name variable for the decorated properties with computed names and keeps the existing property declaration (transforming it to include the assignment to the hoisted variable if necessary).

class X {
    @myDecorator
    [calculateFieldName()] = "hello";
    [calculateMethodName()]() {}
}
// ESNext:
var _a;
class X {
    [_a = calculateFieldName()] = "hello";
    [calculateMethodName()]() {}
}
__decorate([ myDecorator ], X.prototype, _a, void 0);

The ESNext transformer is now responsible for transforming class field declarations. This involves moving the initializers to the constructor and storing computed property names into hoisted variables. Note that this is just for class fields and not for methods, which support computed property names in ES2015. The evaluation order of computed property names must be preserved (see example below where this becomes tricky). This logic was mostly moved from the TypeScript transformer with minor changes to handle the cases where the computed name of a decorated class property was already hoisted into a variable.

class X {
    [calculateFieldName()]: string = "hello";
    [calculateMethodName()]() { console.log('hello'); }
}
// ES2015: (note the placement of the initialization of the field name
//          before the method name to preserve evaluation order)
var _a;
class X {
    constructor() {
        this[_a] = "hello";
    }
    [_a = calculateFieldName(), calculateMethodName()]() { console.log('hello'); }
}

@@ -952,57 +896,60 @@ namespace ts {
* @param node The current class.
* @param isDerivedClass A value indicating whether the class has an extends clause that does not extend 'null'.
Copy link

Choose a reason for hiding this comment

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

update this JSDoc: isDerivedClass param is now gone

@mheiber
Copy link

mheiber commented Oct 4, 2018

@Neuroboy23, continuing discussion on what the changes are in: #10 (comment)

@joeywatts joeywatts force-pushed the class-properties-esnext branch from 946fb65 to dc1c730 Compare October 5, 2018 19:53
Copy link

@Neuroboy23 Neuroboy23 left a comment

Choose a reason for hiding this comment

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

Let's get the transformation of parameter properties into class fields into this PR as well.

@joeywatts joeywatts force-pushed the class-properties-esnext branch from 0ec694f to 0a5bdd2 Compare October 18, 2018 20:09
@joeywatts joeywatts changed the base branch from master to es-private-fields October 18, 2018 20:10
@joeywatts joeywatts force-pushed the class-properties-esnext branch from 51646f7 to 37ae579 Compare October 23, 2018 18:36
@mheiber mheiber closed this Oct 30, 2018
@mheiber mheiber reopened this Oct 30, 2018
@joeywatts joeywatts force-pushed the class-properties-esnext branch from 37ae579 to d23e150 Compare November 7, 2018 18:11
Copy link

@mheiber mheiber left a comment

Choose a reason for hiding this comment

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

🎉 looks good, excited to get this in

Joseph Watts and others added 2 commits November 12, 2018 15:46
Signed-off-by: Joseph Watts <jwatts43@bloomberg.net>
* Add baselines

* Update baselines

Signed-off-by: Joseph Watts <jwatts43@bloomberg.net>
@joeywatts joeywatts force-pushed the class-properties-esnext branch from 86a90df to 861936e Compare November 12, 2018 20:48
@mheiber mheiber merged commit b29c9cf into bloomberg:es-private-fields Nov 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants