Skip to content

Type self-reference in class decorator uses incorrect local variables in generated JS. #9685

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

Closed
EricABC opened this issue Jul 13, 2016 · 8 comments
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@EricABC
Copy link

EricABC commented Jul 13, 2016

TypeScript Version: 2.0.0

The problem is a common occurrence in Angular 2 where forwardRef is often used. It is included in the example, but removing forwardRef and the angular dependency will produce the same issue.

Code

import { forwardRef } from '@angular/core';

declare var Something: any;
@Something({ v: forwardRef(() => Testing123) })
export class Testing123 {}

Expected behavior:

System.register(['@angular/core'], function(exports_1, context_1) {
    "use strict";
    var __moduleName = context_1 && context_1.id;
    var __decorate = (this && this.__decorate) || function (decorators, target, key, desc) {
        var c = arguments.length, r = c < 3 ? target : desc === null ? desc = Object.getOwnPropertyDescriptor(target, key) : desc, d;
        if (typeof Reflect === "object" && typeof Reflect.decorate === "function") r = Reflect.decorate(decorators, target, key, desc);
        else for (var i = decorators.length - 1; i >= 0; i--) if (d = decorators[i]) r = (c < 3 ? d(r) : c > 3 ? d(target, key, r) : d(target, key)) || r;
        return c > 3 && r && Object.defineProperty(target, key, r), r;
    };
    var __metadata = (this && this.__metadata) || function (k, v) {
        if (typeof Reflect === "object" && typeof Reflect.metadata === "function") return Reflect.metadata(k, v);
    };
    var core_1;
    var Testing123;
    return {
        setters:[
            function (core_1_1) {
                core_1 = core_1_1;
            }],
        execute: function() {


*****
// One option that made this work was adding 'let' here, which makes this simple case work.
            let Testing123_1 = class Testing123 {
            };
*****


            let Testing123 = Testing123_1;
            Testing123 = Testing123_1 = __decorate([
                Something({ v: core_1.forwardRef(() => Testing123) }), 
                __metadata('design:paramtypes', [])
            ], Testing123);
            exports_1("Testing123", Testing123);
        }
    }
});

Actual behavior:

System.register(['@angular/core'], function(exports_1, context_1) {
    "use strict";
    var __moduleName = context_1 && context_1.id;
    var __decorate = (this && this.__decorate) || function (decorators, target, key, desc) {
        var c = arguments.length, r = c < 3 ? target : desc === null ? desc = Object.getOwnPropertyDescriptor(target, key) : desc, d;
        if (typeof Reflect === "object" && typeof Reflect.decorate === "function") r = Reflect.decorate(decorators, target, key, desc);
        else for (var i = decorators.length - 1; i >= 0; i--) if (d = decorators[i]) r = (c < 3 ? d(r) : c > 3 ? d(target, key, r) : d(target, key)) || r;
        return c > 3 && r && Object.defineProperty(target, key, r), r;
    };
    var __metadata = (this && this.__metadata) || function (k, v) {
        if (typeof Reflect === "object" && typeof Reflect.metadata === "function") return Reflect.metadata(k, v);
    };
    var core_1;
    var Testing123;
    return {
        setters:[
            function (core_1_1) {
                core_1 = core_1_1;
            }],
        execute: function() {


*****  // Exception raised here, there is no Testing123_1
            Testing123_1 = class Testing123 {
*****


            };
            let Testing123 = Testing123_1;
            Testing123 = Testing123_1 = __decorate([
                Something({ v: core_1.forwardRef(() => Testing123) }), 
                __metadata('design:paramtypes', [])
            ], Testing123);
            exports_1("Testing123", Testing123);
        }
    }
});

The real-world example that worked in 1.8.10 is here:

import { Component, Input, forwardRef, ViewChild, ElementRef, Renderer } from '@angular/core';
import { FormControl, NG_VALUE_ACCESSOR, ControlValueAccessor } from '@angular/forms';

@Component({
    selector: "child",
    templateUrl: 'ChildControl.template.html',
    moduleId: __abcModuleReference(__moduleName),
    providers: [{ provide: NG_VALUE_ACCESSOR, useExisting: forwardRef(() => ChildControl), multi: true }]
})
export class ChildControl implements ControlValueAccessor {
    constructor(private _renderer: Renderer) {
    }

    @ViewChild('inputElement') private _inputElement: ElementRef;

    onChange = (_: any) => { };
    onTouched = () => { };

    writeValue(obj: any): void {
        this._renderer.setElementProperty(this._inputElement.nativeElement, 'value', obj);
    }

    registerOnChange(fn: (_: any) => void): void {
        this.onChange = fn;
    }
    registerOnTouched(fn: () => void): void {
        this.onTouched = fn;
    }

    control = new FormControl();
    @Input() value: string;
}

TSConfig targeting systemjs and es6

{
  "compilerOptions": {
    "noImplicitAny": true,
    "strictNullChecks": true,
    "noImplicitReturns": true,
    "noImplicitThis": true,
    "noUnusedLocals": true,
    "noEmitOnError": false,
    "removeComments": true,
    "target": "es6",
    "inlineSourceMap": true,
    "inlineSources": true,
    "emitDecoratorMetadata": true,
    "experimentalDecorators": true,
    "moduleResolution": "node",
    "module": "system",
    "skipLibCheck": true
  },
  "exclude": [
    "dist",
    "node_modules",
    "gulpfile.ts",
    "gulp"
  ],
  "compileOnSave": true
}
@mhegazy
Copy link
Contributor

mhegazy commented Jul 13, 2016

looks like the same issue as #4521

@mhegazy mhegazy added the Duplicate An existing issue was already created label Jul 13, 2016
@EricABC
Copy link
Author

EricABC commented Jul 13, 2016

@mhegazy I think the issue is different. The previous post refers to eliminating the need to use things like forwardRef. In this issue I am not arguing for that. Instead it shows how forwardRef fails altogether which will have big implications for Angular 2, something that is a breaking change from 1.8.10.

Thanks!

@mhegazy mhegazy added Bug A bug in TypeScript and removed Duplicate An existing issue was already created labels Jul 13, 2016
@mhegazy mhegazy added this to the TypeScript 2.0.1 milestone Jul 13, 2016
@mhegazy
Copy link
Contributor

mhegazy commented Jul 13, 2016

i see the issue now. thanks for the clarification. this is a bug that we should fix.

@EricABC
Copy link
Author

EricABC commented Jul 13, 2016

Awesome, thanks!

@EricABC
Copy link
Author

EricABC commented Jul 14, 2016

FYI this is not limited to decorators it turns out, but any self reference such as accessing a static member of the same type. Simple work-around is to declare a module local variable, use it in place of the type reference, then set the variable to the type underneath the class.

@yuit
Copy link
Contributor

yuit commented Jul 14, 2016

@EricABC could you give an example of what you mean by this, " any self reference such as accessing a static member of the same type." do you mean something like:

declare var Something1: any;

@Something1
export class MyClass {
    static prop0 = " hi"
    static prop1 = () => MyClass.prop0
}

@EricABC
Copy link
Author

EricABC commented Jul 14, 2016

Sure, here is one:

export class Widget {
    static color: string = "green";
    doStuff(): void {
        console.log(Widget.color);
    }
}

A simple work-around is this:

var typeRefThingy: any;
export class Widget {
    static color: string = "green";
    doStuff(): void {
        console.log(typeRefThingy.color);
    }
}
typeRefThingy = Widget;

Any occurrence of Widget that is not after its definition will result in the same issue.
Note, the decorator was how I first encountered the issue, so the title is a bit narrow in scope.

@yuit
Copy link
Contributor

yuit commented Jul 15, 2016

@EricABC Thanks for clarification. I can't repo your second example though with following configuration --t es6 -m system :

Let me know if I miss something here

original

export class Widget {
    static color: string = "green";
    doStuff(): void {
        console.log(Widget.color);
    }
}

output; this looks correct

System.register([], function(exports_1, context_1) {
    "use strict";
    var __moduleName = context_1 && context_1.id;
    var Widget;
    return {
        setters:[],
        execute: function() {
            Widget = class Widget {
                doStuff() {
                    console.log(Widget.color);
                }
            };
            Widget.color = "green";
            exports_1("Widget", Widget);
        }
    }
});

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

3 participants