Skip to content

decoratorMetadataEmit not compatible with Babel's 6-to-5 #2836

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
alexeagle opened this issue Apr 20, 2015 · 12 comments · Fixed by #3063
Closed

decoratorMetadataEmit not compatible with Babel's 6-to-5 #2836

alexeagle opened this issue Apr 20, 2015 · 12 comments · Fixed by #3063
Assignees
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@alexeagle
Copy link
Contributor

I believe it's expected that tsc -t es6 should be externally transpilable to ES5 (eg with Babel)

See https://gist.github.com/alexeagle/24243fbb87dbd8bad6f1

Babel replaces the __decorate line this.__decorate with undefined.__decorate so that gives a runtime error. Uncaught TypeError: Cannot read property '__decorate' of undefined app_bin.js:7

Note I also tried this with Closure Compiler ES6->ES5, and had a different problem because of this line in the emit:
Object.defineProperty(App, "name", { value: "App", configurable: true });
in ES6 spec, new objects are configurable:true by default, but in mainline Chrome it is configurable:false by default so this is also a runtime error. For this case, it leads me to wonder if ES5->ES6 transpilers should be expected to translate API incompatibilities in addition to new ES6 syntax.

@mhegazy
Copy link
Contributor

mhegazy commented Apr 20, 2015

I believe the core of the issue is that the emit for this.__decorate is incompatible with ES6 module semantics (i.e. use of this). this is the same issue as #2811.

I do not have enough context for Object.defineProperty(App, "name", { value: "App", configurable: true }); can you elaborate.

@alexeagle
Copy link
Contributor Author

In current chrome, here is an interactive session:

var f = function(){}
undefined
Object.getOwnPropertyDescriptor(f, "name");
Object {value: "", writable: false, enumerable: false, configurable: false}
Object.defineProperty(f, "name", {value: "name", configurable: true});
VM583:2 Uncaught TypeError: Cannot redefine property: name

@rbuckton
Copy link
Contributor

@mhegazy, we have to manually assign the name of the class that has decorators due to the double-bind issue with class declarations. The problem is that the "name" property of Function has [[Configurable]]: true in ES6, but has [[Configurable]]: false in ES5 Chrome, as well as non-strict contexts in Chrome Canary with ES6 features enabled.

@alexeagle Given Chrome's update cycle, how likely would it be that they make Function#name configurable by default before ES6 support ships?

@alexeagle
Copy link
Contributor Author

I have no idea. But for my part, as long as this is an isolated problem
with external tools doing the 6-to-5 lowering, this is a low priority issue
and can wait for at least a few weeks.

On Mon, Apr 20, 2015 at 3:16 PM Ron Buckton [email protected]
wrote:

@mhegazy https://github.com/mhegazy, we have to manually assign the
name of the class that has decorators due to the double-bind issue with
class declarations. The problem is that the "name" property of Function has
[[Configurable]]: true in ES6, but has [[Configurable]]: false in ES5
Chrome, as well as non-strict contexts in Chrome Canary with ES6 features
enabled.

@alexeagle https://github.com/alexeagle Given Chrome's update cycle,
how likely would it be that they make Function#name configurable by default
before ES6 support ships?


Reply to this email directly or view it on GitHub
#2836 (comment)
.

@mhegazy mhegazy added the Bug A bug in TypeScript label Apr 20, 2015
@mhegazy mhegazy added this to the TypeScript 1.5 milestone Apr 20, 2015
@rbuckton
Copy link
Contributor

@alexeagle, I could change this from Object.defineProperty to Reflect.defineProperty, assuming the polyfill babel uses for its runtime includes Reflect.defineProperty (which I believe it does).

Reflect.defineProperty does not throw if the property has [[Configurable]]: false.

@alexeagle
Copy link
Contributor Author

I don't think this would help us in Closure, their runtime polyfill seems
to just be a couple of function declarations.
Let's revisit if this doesn't make it into mainline Chrome soon.

On Tue, Apr 21, 2015 at 11:44 AM Ron Buckton [email protected]
wrote:

@alexeagle https://github.com/alexeagle, I could change this from
Object.defineProperty to Reflect.defineProperty, assuming the polyfill
babel uses for its runtime includes Reflect.defineProperty (which I
believe it does).

Reflect.defineProperty does not throw if the property has
[[Configurable]]: false.


Reply to this email directly or view it on GitHub
#2836 (comment)
.

@mhegazy
Copy link
Contributor

mhegazy commented Apr 23, 2015

Fixed in #2897

@mhegazy mhegazy closed this as completed Apr 23, 2015
@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Apr 23, 2015
@jamiewinder
Copy link

FYI, I'm still seeing this on the 1.5 beta.

@mhegazy
Copy link
Contributor

mhegazy commented May 1, 2015

@jamiewinder can you provide more details?

this input:

function dec() { }

@dec
export class Foo {

}

on 1.5-beta gives me:

if (typeof __decorate !== "function") __decorate = function (decorators, target, key, desc) {
    if (typeof Reflect === "object" && typeof Reflect.decorate === "function") return Reflect.decorate(decorators, target, key, desc);
    switch (arguments.length) {
        case 2: return decorators.reduceRight(function(o, d) { return (d && d(o)) || o; }, target);
        case 3: return decorators.reduceRight(function(o, d) { return (d && d(target, key)), void 0; }, void 0);
        case 4: return decorators.reduceRight(function(o, d) { return (d && d(target, key, o)) || o; }, desc);
    }
};
function dec() { }
var Foo = (function () {
    function Foo() {
    }
    Foo = __decorate([
        dec
    ], Foo);
    return Foo;
})();

@jamiewinder
Copy link

Here's a basic reproduction, which uses TypeScript 1.5-beta, ES6 output, Babel, and SystemJS together:

index.html

<script src="system.js"></script>
<script>
    System.transpiler = "babel";
    System.babelOptions = {
        modules: "system"
    };
    System.import("app");
</script>

app.ts

import { DecoratedClass } from './decoratedclass';

var dc = new DecoratedClass();
var dummy = dc.name;

DecoratedClass.ts

import { TestDecorator } from './testdecorator';

export class DecoratedClass {
    @TestDecorator()
    public get name(): string {
        return "This is a test";
    }
}

TestDecorator.ts

export function TestDecorator() {
    return (target: any, key: string, props: PropertyDescriptor) => {
        var getter = props.get;

        props.get = function () {
            var val = getter();
            console.log(val);
            return val;
        };
    };
}

The expected result is that the 'This is a test' is written to the console (by the decorator). If I build as ES5 using a CommonJS module system, then it does exactly that. However, if I build to ES6, then I get an error:

Potentially unhandled rejection [2] TypeError: Error evaluating decoratedclass
Cannot read property '__decorate' of undefined
    at execute (http://localhost:31818/decoratedclass.js!eval:15:35)

Let me know if you need more information.

@mhegazy mhegazy reopened this May 1, 2015
@mhegazy mhegazy modified the milestones: TypeScript 1.5.2, TypeScript 1.5 May 1, 2015
@mhegazy mhegazy removed the Fixed A PR has been merged for this issue label May 1, 2015
@mhegazy
Copy link
Contributor

mhegazy commented May 1, 2015

@rbuckton looks like "use strict" issue

@mhegazy mhegazy closed this as completed May 5, 2015
@mhegazy mhegazy added the Fixed A PR has been merged for this issue label May 5, 2015
@alexeagle
Copy link
Contributor Author

I don't think the original issue is fixed. It's still the case that es6 emit produces Object.defineProperty which only works in Chrome Canary, not Chrome.

We now need a proper fix as we are starting to use typescript to compile the decorator parts of Angular.
Maybe what @rbuckton suggested in #2836 (comment) ?

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

Successfully merging a pull request may close this issue.

4 participants