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

Decorator type metadata #2589

Merged
merged 18 commits into from Apr 6, 2015

Conversation

Projects
None yet
4 participants
@rbuckton
Copy link
Member

commented Apr 2, 2015

Resolves #2577

Adds support behind an experimental compiler option to emit design-time type metadata for decorated declarations in source.

  • Adds a __metadata helper that depends on a polyfill for a proposed Reflect.metadata decorator.
  • Adds a __param helper decorator used to apply parameter decorators after metadata is applied and before the method's decorators are run.
  • Emits calls to __metadata for class and member (property and method) decorators
  • For a class declaration, emits the type metadata for constructor parameters of the constructor with a body.
  • For a method declaration, emits the type metadata for the member, its parameters, and its return type.
  • For an accessor or property declaration, emits the type metadata for the member.
  • Adds compiler option to enable experimental metadata support (--emitDecoratorMetadata).

A few notes on metadata:

  • Type metadata uses the metadata key "design:type".
  • Parameter type metadata uses the metadata key "design:paramtypes".
  • Return type metadata uses the metadata key "design:returntype".

@rbuckton rbuckton changed the title Decorators types Decorator type metadata Apr 2, 2015

rbuckton added some commits Apr 2, 2015

//
function emitDecoratorArray(decorators: NodeArray<Decorator>, writeComma: boolean, prefix?: string, postfix?: string): boolean {
if (decorators) {
let decoratorCount = decorators ? decorators.length : 0;

This comment has been minimized.

Copy link
@mhegazy

mhegazy Apr 2, 2015

you already check for decorators is truthy.

function emitDecoratorArray(decorators: NodeArray<Decorator>, writeComma: boolean, prefix?: string, postfix?: string): boolean {
if (decorators) {
let decoratorCount = decorators ? decorators.length : 0;
for (let i = 0; i < decoratorCount; i++) {

This comment has been minimized.

Copy link
@mhegazy

mhegazy Apr 2, 2015

why not emitList()?

This comment has been minimized.

Copy link
@rbuckton

rbuckton Apr 2, 2015

Author Member

emitList emits the provided node array. I don't want to emit the decorators array, I want to emit the expression of each decorator in the array with the correct source maps. I also don't want to have an emitDecorator that just emits the expression of the decorator, so that if/when we add an ES7 or edge script target in the future I would be using emitDecorator to emit the decorator as is.

let decoratorCount = decorators ? decorators.length : 0;
for (let i = 0; i < decoratorCount; i++) {
if (writeComma) {
write(",");

This comment has been minimized.

Copy link
@mhegazy

mhegazy Apr 2, 2015

should have a space after the ", ". another reason to use emitList when possible.

This comment has been minimized.

Copy link
@rbuckton

rbuckton Apr 2, 2015

Author Member

We don't add the space in emitList if we emit each item on a separate line. If I wanted to use emitList I'd have to add optional parameters to handle the special cases for decorator emit.

case SyntaxKind.GetAccessor:
case SyntaxKind.SetAccessor:
case SyntaxKind.PropertyDeclaration:
return true;

This comment has been minimized.

Copy link
@mhegazy

mhegazy Apr 2, 2015

should not this check if ti also has a decorator?

This comment has been minimized.

Copy link
@rbuckton

rbuckton Apr 2, 2015

Author Member

This is only used by emitSerializedTypeMetadata, which is only called for a decorator. Adding the check would add unnecessary overhead.

@@ -8734,6 +8745,42 @@ module ts {
}
}

/** Checks a type reference node as an expression. */

This comment has been minimized.

Copy link
@mhegazy

mhegazy Apr 2, 2015

can you add a comment on why we are doing this.

@@ -8742,18 +8789,32 @@ module ts {

switch (node.kind) {
case SyntaxKind.ClassDeclaration:
var constructor = getFirstConstructorWithBody(<ClassDeclaration>node);
if (constructor) {
checkParameterTypeAnnotationsAsExpressions(constructor);

This comment has been minimized.

Copy link
@mhegazy

mhegazy Apr 2, 2015

We only need to do that if the metadata flag is passed. correct?

This comment has been minimized.

Copy link
@rbuckton

rbuckton Apr 2, 2015

Author Member

Correct. I've made a change I'll push up shortly with this.

@@ -11445,6 +11510,199 @@ module ts {
return undefined;
}

/** Serializes an EntityName (with substitutions) to an appropriate JS constructor value. Used by the `@type`, `@paramtypes`, and `@returntype` decorators. */

This comment has been minimized.

Copy link
@mhegazy

mhegazy Apr 2, 2015

@__metadata decorators

function checkParameterTypeAnnotationsAsExpressions(node: FunctionLikeDeclaration) {
// ensure all type annotations with a value declaration are checked as an expression
if (node) {
forEach(node.parameters, checkTypeAnnotationAsExpression);

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Apr 2, 2015

Contributor

this will stop with the first checkTypeAnnotationAsExpression that returns something 'truthy'. Is that what you wanted?

This comment has been minimized.

Copy link
@rbuckton

rbuckton Apr 2, 2015

Author Member

checkTypeAnnotationAsExpression returns undefined, so generally this is fine. I have been periodically migrating these to for..of since its available in the compiler now, though it wasn't when this was originally written.

*/
function checkTypeAnnotationAsExpression(node: AccessorDeclaration | PropertyDeclaration | ParameterDeclaration | MethodDeclaration) {
switch (node.kind) {
case SyntaxKind.PropertyDeclaration: return checkTypeNodeAsExpression((<PropertyDeclaration>node).type);

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Apr 2, 2015

Contributor

this is kinda 'ew'. You're returning something that is 'void'. It makes it seem as if checkTypeAnnotationAsExpression will be non-void.

function serializeTypeReferenceNode(node: TypeReferenceNode, getGeneratedNameForNode: (Node: Node) => string): string | string[] {
// serialization of a TypeReferenceNode uses the following rules:
//
// * The serialized type of a TypeReference that is `void` is "void 0".

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Apr 2, 2015

Contributor

Are we locked into this in the future. Will changing these constitute 'breaking changes'?

This comment has been minimized.

Copy link
@rbuckton

rbuckton Apr 2, 2015

Author Member

There may be breaking changes in the near future, however this is currently an experimental feature.

// * The serialized type of any other type is "Object".
let type = getTypeFromTypeReference(node);
if (type.flags & TypeFlags.Void) {
return "void 0";

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Apr 2, 2015

Contributor

Why 'void 0' out of curiosity?

This comment has been minimized.

Copy link
@rbuckton

rbuckton Apr 2, 2015

Author Member

That's part of the serialization rules that @mhegazy and I worked out. Currently, every serialized type points to some constructor function, except for void, which has no related constructor function. We emit void types as undefined, though we use void 0 here to emit undefined, just as we do elsewhere within the compiler.

case SyntaxKind.UnionType:
case SyntaxKind.AnyKeyword:
default:
break;

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Apr 2, 2015

Contributor

Possibly assert here that somethign was missed. That way if we add a new type kind in the future we know we have to update this.

//
// For rules on serializing type annotations, see `serializeTypeNode`.
switch (node.kind) {
case SyntaxKind.ClassDeclaration: return "Function";

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Apr 2, 2015

Contributor

Is this appropriate if you're doing ES6 emit?

This comment has been minimized.

Copy link
@rbuckton

rbuckton Apr 2, 2015

Author Member

Generally, the serialized type of a declaration is the function constructor that would satisfy an instanceof check (except for primitive types, as they are serialized as their boxed constructor type).

class C { }
C instanceof Function; // true
for (var i = 0; i < parameterCount; i++) {
if (parameters[i].dotDotDotToken) {
var parameterType = parameters[i].type;
if (parameterType.kind === SyntaxKind.ArrayType) {

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Apr 2, 2015

Contributor

So we're doing this stuff syntactically only? Is that ok? Should we consider using the type information isntead?

rbuckton added a commit that referenced this pull request Apr 6, 2015

Merge pull request #2589 from Microsoft/decorators_types
Experimental support for decorator type metadata. 
NOTE: Requires a polyfill for `Reflect.metadata` which has not yet been considered by TC39 for ES7.

@rbuckton rbuckton merged commit e195d89 into master Apr 6, 2015

1 check passed

continuous-integration/travis-ci/push The Travis CI build passed
Details

@rbuckton rbuckton deleted the decorators_types branch Apr 6, 2015

@basarat basarat referenced this pull request Apr 6, 2015

Closed

Decorators #2249

@mhegazy mhegazy referenced this pull request May 18, 2015

Closed

Expose new compiler flags in the .targets file #3034

14 of 14 tasks complete

@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.