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

Partial support decorators when targeting ES3 #4681

Closed
Koloto opened this issue Sep 7, 2015 · 11 comments
Closed

Partial support decorators when targeting ES3 #4681

Koloto opened this issue Sep 7, 2015 · 11 comments
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@Koloto
Copy link

Koloto commented Sep 7, 2015

Method/accessor decorators use defineProperty and getOwnPropertyDescriptor, which are not allowed in ES3. But class and property decorators don't use them. So class/property decorators can be allowed when targeting ES3.

It would be helpful. E.g. I must support old browsers and I use the property like methods instead of pure properties:

value(v?) {
    if (arguments.length) {
        // setter
    } else {
        // getter
    }
}

I have the factory function that creates such method:

interface Accessor<T> {
    (): T;
    (v: T): void;
}

function accessor<T>(propName: string): Accessor<T>;
function accessor<T>(propName: string) {
    return function (v?: T) {
        // if (arguments.length) and bla-bla-bla
    }
}

Currently I have to set such properties in the class prototype explicitly:

class Foo {
    value: Accessor<string>;
}
Foo.prototype.value = accessor<string>("value");

With decorators I could write more cleaner code:

function Accessor(proto, propName: string) {
    proto[propName] = accessor(propName);
}

class Foo {
    @Accessor
    value: Accessor<string>;
}

Also there is no need to duplicate the property name ("value" in the example above).

@Koloto Koloto changed the title Partially support decorators when targeting ES3 Partial support decorators when targeting ES3 Sep 7, 2015
@mhegazy mhegazy added the Suggestion An idea for TypeScript label Sep 8, 2015
@mhegazy
Copy link
Contributor

mhegazy commented Sep 8, 2015

The problem is the __decorate helper uses ES5 features like reduceRight. so any proposal here would need to address that as well.

@mhegazy mhegazy added the Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. label Sep 8, 2015
@xLama
Copy link

xLama commented Sep 8, 2015

@mhegazy
Copy link
Contributor

mhegazy commented Sep 8, 2015

TypeScript does not emit, or depend on any polyfills. if you do for your code, and include polyfills you manage them separately. for helper code emitted by the compiler there is no dependency on any polyfills currently, nor this is intended.

__decorate helper will be emitted in every file that uses decorators. we try to keep any helper code as small as and as efficient as possible, since we are polluting user code.

Also we do not emit different versions of the helpers based on the target, to avoid breaking user code from an imported library built with a different target, but loaded first.

so a proposal for this should include the shape of the __decorate that does not use reduceRight, which will either be a for loop with a call to function, or a call to an additional helper function.

Personally, i would say the shape the helper is important, as again we emit it in every file. and since day 1, typescript strives to emit clean, idiomatic, JavaScript that would looks like the code you would write by hand if you were not using a compiler.

@Koloto
Copy link
Author

Koloto commented Sep 9, 2015

Proposal for __decorate helper without reduceRight:

var __decorate = (this && this.__decorate) || function (decorators, target, key, desc) {
    if (typeof Reflect === "object" && typeof Reflect.decorate === "function") return Reflect.decorate(decorators, target, key, desc);
    for (var i = decorators.length - 1, d, r; i >= 0; i--) if (d = decorators[i]) switch (arguments.length) {
        case 2: r = target = d(target) || target; break;
        case 3: d(target, key); break;
        case 4: r = desc = d(target, key, desc) || desc; break;
    }
    return r;
};

This version is a bit shorter than the standard helper:)

@Koloto
Copy link
Author

Koloto commented Sep 10, 2015

More safe version (always returns the result, even if decorators is an empty array):

var __decorate = (this && this.__decorate) || function (decorators, target, key, desc) {
    if (typeof Reflect === "object" && typeof Reflect.decorate === "function") return Reflect.decorate(decorators, target, key, desc);
    for (var i = decorators.length - 1, d; i >= 0; i--) if (d = decorators[i]) switch (arguments.length) {
        case 2: target = d(target) || target; break;
        case 3: d(target, key); break;
        case 4: desc = d(target, key, desc) || desc; break;
    }
    return desc || target;
};

@mhegazy What do you think? Is it ok?

@mhegazy
Copy link
Contributor

mhegazy commented Sep 10, 2015

@rbuckton any comments?

@rbuckton
Copy link
Member

@Koloto We initially did something similar, but simplified the __decorate helper when we decided to only support decorators for ES5+.

If we do decide to support ES3, we would likely support method decorators in the same fashion as property decorators (e.g. without the property descriptor).

@rbuckton
Copy link
Member

I tweaked @Koloto's version a bit to avoid repeated lookups on arguments.length and adjusted the return value:

var __decorate = (this && this.__decorate) || function (decorators, target, key, desc) {
    if (typeof Reflect === "object" && typeof Reflect.decorate === "function") return Reflect.decorate(decorators, target, key, desc);
    for (var i = decorators.length - 1, argc = arguments.length, d; i >= 0; i--) if (d = decorators[i]) switch (argc) {
        case 2: target = d(target) || target; break;
        case 3: d(target, key); break;
        case 4: desc = d(target, key, desc) || desc; break;
    }
    if (argc !== 3) return desc || target;
};

@rbuckton
Copy link
Member

Further reduced:

var __decorate = (this && this.__decorate) || function (decorators, target, key, desc) {
    if (typeof Reflect === "object" && typeof Reflect.decorate === "function") return Reflect.decorate(decorators, target, key, desc);
    for (var i = decorators.length - 1, c = arguments.length, r = c < 3 ? target : desc, d; i >= 0; i--) 
        if (d = decorators[i]) r = (c < 3 ? d(r) : c > 3 ? d(target, key, r) : void d(target, key)) || r;
    return r;
};

@Koloto
Copy link
Author

Koloto commented Sep 10, 2015

@rbuckton

If we do decide to support ES3, we would likely support method decorators in the same fashion as property decorators (e.g. without the property descriptor).

It would be great to support method decorators in ES3 also. Can I hope to see this feature in v1.6? :) I suppose, no. But support just class and property decorators in ES3 seems much easier. It would be a good start.

@rbuckton
Copy link
Member

rbuckton commented Oct 2, 2015

This should be addressed as of #4741.

Decorators are now allowed when targeting ES3. In this case, all decorators on members of a class are treated in the same fashion as a decorator on a property declaration. A descriptor for the declaration will neither be provided to nor consumed by the __decorate helper in this case.

Decorators for ES5 and higher should be unaffected (although the actual emit for each decorator has changed slightly).

@rbuckton rbuckton closed this as completed Oct 2, 2015
@mhegazy mhegazy added Fixed A PR has been merged for this issue and removed Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. labels Oct 3, 2015
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

4 participants