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

Keep class properties in ESNext target #10

Merged
merged 2 commits into from
Nov 15, 2018
Merged

Keep class properties in ESNext target #10

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
75af3b7
Move class property transformation into ESNext transformer
Sep 20, 2018
861936e
Baseline changes for ESNext Class Property Transformation (#4)
joeywatts Sep 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 13 additions & 11 deletions src/compiler/binder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3189,8 +3189,7 @@ namespace ts {
// A ClassDeclaration is ES6 syntax.
transformFlags = subtreeFlags | TransformFlags.AssertES2015;

// A class with a parameter property assignment, property initializer, computed property name, or decorator is
// TypeScript syntax.
// A class with a parameter property assignment or decorator is TypeScript syntax.
// An exported declaration may be TypeScript syntax, but is handled by the visitor
// for a namespace declaration.
if ((subtreeFlags & TransformFlags.ContainsTypeScriptClassSyntax)
Expand All @@ -3213,8 +3212,7 @@ namespace ts {
// A ClassExpression is ES6 syntax.
let transformFlags = subtreeFlags | TransformFlags.AssertES2015;

// A class with a parameter property assignment, property initializer, or decorator is
// TypeScript syntax.
// A class with a parameter property assignment or decorator is TypeScript syntax.
if (subtreeFlags & TransformFlags.ContainsTypeScriptClassSyntax
|| node.typeParameters) {
transformFlags |= TransformFlags.AssertTypeScript;
Expand Down Expand Up @@ -3310,7 +3308,6 @@ namespace ts {
|| hasModifier(node, ModifierFlags.TypeScriptModifier)
Copy link

@mheiber mheiber Sep 27, 2018 •
edited
Loading

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 •
edited
Loading

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'); }
}

|| node.typeParameters
|| node.type
|| (node.name && isComputedPropertyName(node.name)) // While computed method names aren't typescript, the TS transform must visit them to emit property declarations correctly
|| !node.body) {
transformFlags |= TransformFlags.AssertTypeScript;
}
Expand Down Expand Up @@ -3341,7 +3338,6 @@ namespace ts {
if (node.decorators
|| hasModifier(node, ModifierFlags.TypeScriptModifier)
|| node.type
|| (node.name && isComputedPropertyName(node.name)) // While computed accessor names aren't typescript, the TS transform must visit them to emit property declarations correctly
|| !node.body) {
transformFlags |= TransformFlags.AssertTypeScript;
}
Expand All @@ -3356,12 +3352,16 @@ namespace ts {
}

function computePropertyDeclaration(node: PropertyDeclaration, subtreeFlags: TransformFlags) {
// A PropertyDeclaration is TypeScript syntax.
let transformFlags = subtreeFlags | TransformFlags.AssertTypeScript;
// A PropertyDeclaration is ESnext syntax.
let transformFlags = subtreeFlags | TransformFlags.AssertESNext;

// If the PropertyDeclaration has an initializer or a computed name, we need to inform its ancestor
// so that it handle the transformation.
if (node.initializer || isComputedPropertyName(node.name)) {
// Decorators, TypeScript-specific modifiers, and type annotations are TypeScript syntax.
if (some(node.decorators) || hasModifier(node, ModifierFlags.TypeScriptModifier) || node.type) {
transformFlags |= TransformFlags.AssertTypeScript;
}

// Hoisted variables related to class properties should live within the TypeScript class wrapper.
if (isComputedPropertyName(node.name) || (hasStaticModifier(node) && node.initializer)) {
transformFlags |= TransformFlags.ContainsTypeScriptClassSyntax;
}

Expand Down Expand Up @@ -3762,6 +3762,8 @@ namespace ts {
break;

case SyntaxKind.ComputedPropertyName:
// Computed property names are transformed by the ESNext transformer.
transformFlags |= TransformFlags.AssertESNext;
// Even though computed property names are ES6, we don't treat them as such.
joeywatts marked this conversation as resolved.
Show resolved Hide resolved
// This is so that they can flow through PropertyName transforms unaffected.
// Instead, we mark the container as ES6, so that it can properly handle the transform.
Expand Down
442 changes: 438 additions & 4 deletions src/compiler/transformers/esnext.ts
View file
Open in desktop

Large diffs are not rendered by default.

493 changes: 146 additions & 347 deletions src/compiler/transformers/ts.ts
View file
Open in desktop

Large diffs are not rendered by default.

80 changes: 80 additions & 0 deletions src/compiler/transformers/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,47 @@ namespace ts {
isIdentifier(expression);
}

/**
* A simple inlinable expression is an expression which can be copied into multiple locations
* without risk of repeating any sideeffects and whose value could not possibly change between
* any such locations
*/
export function isSimpleInlineableExpression(expression: Expression) {
return !isIdentifier(expression) && isSimpleCopiableExpression(expression) ||
isWellKnownSymbolSyntactically(expression);
}

/**
* Adds super call and preceding prologue directives into the list of statements.
*
* @param ctor The constructor node.
* @param result The list of statements.
* @param visitor The visitor to apply to each node added to the result array.
* @returns index of the statement that follows super call
joeywatts marked this conversation as resolved.
Show resolved Hide resolved
*/
export function addPrologueDirectivesAndInitialSuperCall(ctor: ConstructorDeclaration, result: Statement[], visitor: Visitor): number {
if (ctor.body) {
const statements = ctor.body.statements;
// add prologue directives to the list (if any)
const index = addPrologue(result, statements, /*ensureUseStrict*/ false, visitor);
if (index === statements.length) {
// list contains nothing but prologue directives (or empty) - exit
return index;
}

const statement = statements[index];
if (statement.kind === SyntaxKind.ExpressionStatement && isSuperCall((<ExpressionStatement>statement).expression)) {
result.push(visitNode(statement, visitor, isStatement));
return index + 1;
}

return index;
}

return 0;
}


/**
* @param input Template string input strings
* @param args Names which need to be made file-level unique
Expand All @@ -255,4 +296,43 @@ namespace ts {
return result;
};
}

/**
* Gets all property declarations with initializers on either the static or instance side of a class.
*
* @param node The class node.
* @param isStatic A value indicating whether to get properties from the static or instance side of the class.
*/
export function getInitializedProperties(node: ClassExpression | ClassDeclaration, isStatic: boolean): ReadonlyArray<PropertyDeclaration> {
return filter(node.members, isStatic ? isStaticInitializedProperty : isInstanceInitializedProperty);
}

/**
* Gets a value indicating whether a class element is a static property declaration with an initializer.
*
* @param member The class element node.
*/
export function isStaticInitializedProperty(member: ClassElement): member is PropertyDeclaration & { initializer: Expression; } {
return isInitializedProperty(member) && hasStaticModifier(member);
}
joeywatts marked this conversation as resolved.
Show resolved Hide resolved

/**
* Gets a value indicating whether a class element is an instance property declaration with an initializer.
*
* @param member The class element node.
joeywatts marked this conversation as resolved.
Show resolved Hide resolved
*/
export function isInstanceInitializedProperty(member: ClassElement): member is PropertyDeclaration & { initializer: Expression; } {
return isInitializedProperty(member) && !hasStaticModifier(member);
}

/**
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.

* Gets a value indicating whether a class element is either a static or an instance property declaration with an initializer.
*
* @param member The class element node.
* @param isStatic A value indicating whether the member should be a static or instance member.
*/
export function isInitializedProperty(member: ClassElement): member is PropertyDeclaration & { initializer: Expression; } {
return member.kind === SyntaxKind.PropertyDeclaration
&& (<PropertyDeclaration>member).initializer !== undefined;
}
}
4 changes: 2 additions & 2 deletions tests/baselines/reference/declarationEmitPrivateSymbolCausesVarDeclarationEmit2.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ var C = /** @class */ (function () {
}
return C;
}());
_a = a_1.x;
exports.C = C;
_a = a_1.x;
//// [c.js]
"use strict";
var __extends = (this && this.__extends) || (function () {
Expand Down Expand Up @@ -64,8 +64,8 @@ var D = /** @class */ (function (_super) {
}
return D;
}(b_1.C));
_a = a_1.x;
exports.D = D;
_a = a_1.x;


//// [a.d.ts]
Expand Down
Loading