microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.3k stars 12.39k forks source link

Decorators #2249

Closed rbuckton closed 8 years ago

rbuckton commented 9 years ago

ES7 proposal

The ES7 proposal for decorators can be found here: https://github.com/wycats/javascript-decorators The ES7 proposal serves as the base of this proposal. Below are notes about how the type system

Decorator targets:

Class constructor

@F("color")
@G
class Foo {
}

desugars to:

var Foo = (function () {
    function Foo() {
    }
    Foo = __decorate([F("color"), G], Foo);
    return Foo;
})();

Methods

class Foo {
  @F(color)
  @G
  bar() { }
}

desugars to:

var Foo = (function () {
    function Foo() {
    }
    Foo.prototype.bar = function () {
    };
    Object.defineProperty(Foo.prototype, "bar", __decorate([F(color), G], Foo.prototype, "bar", Object.getOwnPropertyDescriptor(Foo.prototype, "bar")));
    return Foo;
})();

Static method

class Foo {
    @F("color")
    @G
    static sMethod() {}
}

desugars to:

var Foo = (function () {
    function Foo() {
    }
    Foo.sMethod = function () {
    };
    Object.defineProperty(Foo, "sMethod", __decorate([F("color"), G], Foo, "sMethod", Object.getOwnPropertyDescriptor(Foo, "sMethod")));
    return Foo;
})();

Properties

class Foo {
    @F("color")
    @G
    prop: number;
}

desugars to:

var Foo = (function () {
    function Foo() {
    }
    __decorate([F("color"), G], Foo.prototype, "prop");
    return Foo;
})();

Method/Accessor formal parameter

class Foo {
    method(@G a, @F("color") b) {}
}

desugars to:

var Foo = (function () {
    function Foo() {
    }
    Foo.prototype.method = function (a, b) {
    };
    __decorate([G], Foo.prototype, "method", 0);
    __decorate([F("color")], Foo.prototype, "method", 1);
    return Foo;
})();

Where the __decorate is defined as:

var __decorate = this.__decorate || function (decorators, target, key, value) {
    var kind = typeof (arguments.length == 2 ? value = target : value);
    for (var i = decorators.length - 1; i >= 0; --i) {
        var decorator = decorators[i];
        switch (kind) {
            case "function": value = decorator(value) || value; break;
            case "number": decorator(target, key, value); break;
            case "undefined": decorator(target, key); break;
            case "object": value = decorator(target, key, value) || value; break;
        }
    }
    return value;
};

Decorator signatures:

A valid decorator should be:

  1. Assignable to one of the Decorator types (ClassDecorator | PropertyDecorator | MethodDecorator | ParameterDecorator) as described below.
  2. Return a value (in the case of class decorators and method decorator) that is assignable to the decorated value.
declare type ClassDecorator = <TFunction extends Function>(target: TFunction) => TFunction | void;
declare type PropertyDecorator = (target: Object, propertyKey: string | symbol) => void;
declare type MethodDecorator = <T>(target: Object, propertyKey: string | symbol, descriptor: TypedPropertyDescriptor<T>) => TypedPropertyDescriptor<T> | void;
declare type ParameterDecorator = (target: Function, propertyKey: string | symbol, parameterIndex: number) => void;

Notes:

cmichaelgraham commented 9 years ago

crazy cool !!!!

using gulp-typescript to build this repo - (nice work @ivogabe)

gulpfile build-ts command (note the emitDecoratorMetadata option):

gulp.task('build-ts', function () {
    var tsResult = gulp.src([
        './views/*.ts',
        './typings/**/*.d.ts',
        './*.ts'
        ],
        {base: "."})
    .pipe(ts({
         typescript: require('typescript'),
         declarationFiles: false,
         noExternalResolve: true,
         target: "es5",
         module: "amd",
         emitDecoratorMetadata: true
    }));

    return merge([
        tsResult.dts.pipe(gulp.dest('.')),
        tsResult.js.pipe(gulp.dest('.'))
    ]);
});

turns app.ts

import {inject} from 'aurelia-framework';
import {Router} from 'aurelia-router';
import 'bootstrap';
import 'bootstrap/css/bootstrap.css!';

@inject(Router)
export class App {
  public router;
  constructor(router:Router) {
    this.router = router;
    this.router.configure(config => {
      config.title = 'Aurelia';
      config.map([
        { route: ['','welcome'],  moduleId: './welcome',      nav: true, title:'Welcome' },
        { route: 'flickr',        moduleId: './flickr',       nav: true },
        { route: 'child-router',  moduleId: './child-router', nav: true, title:'Child Router' }
      ]);
    });
  }
}

into app.js

var __decorate = this.__decorate || (typeof Reflect === "object" && Reflect.decorate) || function (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);
    }
};
var __metadata = this.__metadata || (typeof Reflect === "object" && Reflect.metadata) || function () { };
define(["require", "exports", 'aurelia-framework', 'aurelia-router', 'bootstrap', 'bootstrap/css/bootstrap.css!'], function (require, exports, aurelia_framework_1, aurelia_router_1, , ) {
    var App = (function () {
        function App(router) {
            this.router = router;
            this.router.configure(function (config) {
                config.title = 'Aurelia';
                config.map([
                    { route: ['', 'welcome'], moduleId: './welcome', nav: true, title: 'Welcome' },
                    { route: 'flickr', moduleId: './flickr', nav: true },
                    { route: 'child-router', moduleId: './child-router', nav: true, title: 'Child Router' }
                ]);
            });
        }
        App = __decorate([
            aurelia_framework_1.inject(aurelia_router_1.Router), 
            __metadata('design:paramtypes', [aurelia_router_1.Router])
        ], App);
        return App;
    })();
    exports.App = App;
});

of special interest is the emiited type metadata about the constructor parameters:

        App = __decorate([
            aurelia_framework_1.inject(aurelia_router_1.Router), 
            __metadata('design:paramtypes', [aurelia_router_1.Router])
        ], App);

so in theory, you could alter the inject decorator function to inject the proper classes without specifying them in the inject, but instead specifying the types in the constructor :)

EisenbergEffect commented 9 years ago

@cmichaelgraham We can easily enable this today without altering the framework. The DI library from Aurelia has a hook container.addParameterInfoLocator. You pass it a function that can take a type and return its dependencies. For our next release (next week), we can add this to the core configuration though so it's easy for TypeScript developers to turn it on. I would have put it in this week's release, but I hadn't realized that this was changed yet.

cmichaelgraham commented 9 years ago

@EisenbergEffect brilliant !! :+1:

mweststrate commented 9 years ago

I used annotations to mark properties of an object as observable, which transforms a primitive into an observable object (for those interested, https://github.com/mweststrate/MOBservable/commit/8cc7fc0e20c000db660037c8b5c9d944fe4155d9).

However, especially for properties, it felt a bit unnatural that the annotations are applied to the prototype, and not in the constructor of a class itself, this means that it is hard to either get the the initial value, or even the this. The work around for that was to create a property, and in its getter/setter perform the actual logic that the annotation was supposed to do, since the this and the value are available then.

Besides that, awesome feature!

matjaz commented 9 years ago

Using Metadata Reflection API I was able to do a simple properties dependency injection. You can check required changes here https://github.com/matjaz/property-DI/commit/2b4835e100b72d954b57d0e656ea524539ac17eb.

VaclavObornik commented 9 years ago

Shouldn't be metadata decorator in generated code invoked first of all decorators? I wanted to create simple deocrator using class's metadata, but metadata('design:paramtypes', [TYPES....]) is invoked as last, so i can not get these data from Reflect

mhegazy commented 9 years ago

@ufon can you share the code you are looking at?

VaclavObornik commented 9 years ago

Sure, here https://gist.github.com/ufon/5a2fa2481ac412117532

EDIT: my bad, there is some other mistake in my code, cannot get types even after class inicialization

mhegazy commented 9 years ago

@ufon, i am not sure i see the issue then. __metadata is the last item in the decorator list, this means it is the first to execute, and definitely before inject gets to execute.

    House = __decorate([
        inject_1.inject, 
        __metadata('design:paramtypes', [Floor, String])
    ], House);

where __decorate definition uses reduce right to execute the decorators in reverse order.

decorators.reduceRight(function(o, d) { return (d && d(o)) || o; }, target);

This is to match the spec, that decorators (as expressions) are evaluated in order of declaration, but executed in the reverse order, allowing outer decorators to consume results from inner decorators.

VaclavObornik commented 9 years ago

I see, sorry for annoy :) my bad, there is some other mistake in my code, cannot get types even after class inicialization

mhegazy commented 9 years ago

how are you querying for them? and do you have a pollyfill for Reflect.metadata?

VaclavObornik commented 9 years ago

added to the gist... Reflect.getMetadataKeys(House); this results empty array..

VaclavObornik commented 9 years ago

Yes, I'm requiring 'reflect-metadata' package

mhegazy commented 9 years ago

looks like the polyfill loosing it, @rbuckton can you take a look.

VaclavObornik commented 9 years ago

Think I got it... requires are moved after typescript's helper code

var __metadata = this.__metadata || (typeof Reflect === "object" && Reflect.metadata) || function () { };
require('reflect-metadata');

so, in time var __metadata is beeing inicialized, polyfill is not loaded yet

VaclavObornik commented 9 years ago

but i cannot get these requires before the generated code

EDIT: updated the gist.. when single module is compiled, there is no way to load polyfill before __metadata is resolved, because typescript's generated code is always moved to the top of the file

mhegazy commented 9 years ago

ooh.. this is a bigger issue. logged issue #2811 to track it

matjaz commented 9 years ago

Until then you require reflect-metadata in another module (you need two files). see https://github.com/matjaz/property-DI/

Tharaxis commented 9 years ago

There appears to be an invocation inconsistency which makes developing decorators with variable/optional parameters pretty difficult to do. As an example:

@F
prop: number;

@F()
prop: number;

In the former, F gets invoked with the parameters (target, propertyName, propertyDescriptor) while in the latter, F gets invoked with the the parameters () and requires you have an internal function returned which in turn gets invoked with (target, propertyName and propertyDescriptor).

My assumption would have been that both @F and @F() would be equivalent and therefore you can only perform either call if the decorator supports 0 or more parameters.

This is particularly important in the case:

@F  // Performs some default behaviour.
prop: number;

@F({ option: true }) // Performs some configured behaviour.
prop: number;

This can be worked around by just explicitly calling @F() instead of @F but it's easy to make that mistake and then be confused when your decorator doesn't work even though by the looks of it it should.

In this case I would like to do the following:

export function F(options?: any): PropertyDecorator {
    return (target, name) => {
        // do something.
    }
}

and be done with it, but instead I have to do something like the following crude example:

export function F(...args: any[]): any {
   var func = (target, name) => {
      // do something.
   }

   if (args.length === 1) return func;
   else if (args.length === 2) func(args[0], args[1]);
}

Which is a real pain.

mhegazy commented 9 years ago

@Tharaxis this behavior is by design. @F and @F() are not equivalent. @F calls a decorator F, where as @F() calls a decorator factor F with empty argument list, and then applies the resulting decorator.

The problem here is that the compiler compares the signature of the decorator factory F against declare type PropertyDecorator = (target: Object, propertyKey: string | symbol) => void; which ends up to be assignable. i think we need a stronger check here to make sure we catch these issue. I have logged #3246 to fix this. I believe you are doing the correct thing, and the compiler should catch invalid uses of a decorator factory.

Tharaxis commented 9 years ago

@mhegazy I'm not sure why this is the case however, is there some use case that precludes one making @F and @F() equivalent, since ultimately they are both an identical invocation of the decorator function, just one of them happens to also include an invocation to an outer factory function? It seems like the principle of least astonishment is being violated here since this is definitely quite astonishing behaviour and I don't see why there have to be 2 ways to invoking a decorator from the perspective of the API consumer.

While I get that from a JS perspective there is a difference (in one you are passing the reference of the F function and the other you are executing a function and then passing in the reference of its result as the decorator), I'm not sure I understand why one cannot just have decorators that are passed as @F implicitly match (and invoke) the factory as one with an empty parameter list?

mhegazy commented 9 years ago

The compiler has no way to know if it is a factory or a decorator. if you have this signature:

declare function decoratorOrFactory (...args: any[]): any;

Is this a decorator or is it a factory, should it be called with empty arguments or not?

The most the compiler can do is verify that the signature matches when calling it one way or the other, and give errors if it does not.

@rbuckton has a fix for #3246 that should flag the cases where your decorator is not used as a factory, so stay tuned.

Tharaxis commented 9 years ago

@mhegazy, what I'm saying is that the use of a decorator (i.e. @F or @F()) should always be an invocation of factory function F, which in turn returns the appropriate decorator type. There is no top-level representation of a decorator without an encapsulating factory.

In other words the signature of F is always:

declare function F(...args: any[]): ClassDecorator | PropertyDecorator | MethodDecorator | ParameterDecorator;

Or some equivalent.

Then invocation @F is made the equivalent of @F() by the compiler. Since both the signature of @F and @F() must match the factory signature in some form this solves the problem of invocation confusion. There is only one way to invoke a decorator!

Given the following:

declare function F(...args: any[]): PropertyDecorator {
    return (target, name) => {
        // do stuff.
    }
}

@F()
property: number;

and

declare function F(target, name) { // Matches PropertyDecorator
    // do stuff.
}

@F
property: number;

They do the same thing but I have to declare them differently! However, declaring F so that it can exist both ways is really cumbersome and makes creating consistent parameters and documentation impossible. You're either choosing that your decorator needs to use a factory or not, and your API consumer has to know this distinction before hand to make use of it.

DavidSouther commented 9 years ago

@Tharaxis If I'm looking at a library providing F that will be used as a decorator, I absolutely expect @F and @F() to be different things. Just like I would expect C and C() to be different things - the first references a value, the second invokes a value. @F applies a decorator, @F() invokes a function that will create a decorator. Those are not, and should not be, the same operation.

Tharaxis commented 9 years ago

@DavidSouther is there any valid reason why this cannot or should not be the case? There is no functional difference between an invocation of @F and @F() but there is a pretty big difference between how they are defined and documented, and it adds additional complexity to invocation that should be unnecessary.

My proposal is that there should be not be two ways to define a decorator because there is no need, and there should be no need to define a decorator more than one way.

mhegazy commented 9 years ago

There is no functional difference between an invocation of @F and @F()

I think this is the main point of contention. i do not think this is true. F is a reference to a function, F() is a reference to the return value of F when called with an empty argument set (i.e. F.call(this, [])); and these are two functionally, and conceptually different things.

DavidSouther commented 9 years ago

@Tharaxis @F and @F() are different things. There is a difference in how they are used, a difference in how they are documented, and differences in their invocation that is absolutely necessary.

Let me ask it a different way: why must every decorator function be a factory? Using your proposal, it would be impossible to have a simple decorator.

Tharaxis commented 9 years ago

@mhegazy while I will concede that the latter option (@F()) does result in the generation of a functional closure and additional overhead through the creation of isolated (and duplicated) decorator functions and the @F invocation essentially performs against a shared decorator function reference, I would say that the isolation through closure would be "safer" and more consistent, reducing the likelihood of surprises.

@DavidSouther Your question depends on what you define as a simple decorator. Nothing precludes one having a simple decorator in the form of:

declare function F(): PropertyDecorator {
    return (target, name) => {
        // do stuff
    }
}

I find this pretty minimally invasive since it's only 2 lines more than the "simple syntax" and defines more obviously what the available decorator invocations are. Additionally, since there is then only a single way to define a decorator (and let us not discount the value of consistency), you'll probably take roughly another 2 seconds to whip this out compared to the "simple syntax".

Additionally a more problematic issue is as follows:

declare function F(options?: any): PropertyDecorator {
    return (target, name) => {
        // do stuff
    }
}

@F()
property1: number;

@F
property2: number;

@F({ option: true })
property3: number;

One of these things is not like the other. The use of @F will not function even though at face value it definitely should. Keep in mind, pretend I am someone who is not aware of what the underlying declaration of F looks like, but instead just know that F exists and that it can take an optional set of arguments. The chances of me making a mistake with @F is not trivial under the current mechanism. This puts a large onus on the developer/documenter to make sure that the consumer is aware that @F will not work (but maybe @C will because it's "different" somehow).

If I want a "one size fits all" decorator, where the consumer doesn't have to worry, I have to do this ghastly thing:

declare function F(...args: any[]): any {
    var decorator = (target, name) => {
        // do stuff
    }

    // Heaven forbid your decorator formal parameter list also can take 2 parameters.
    return (args.length === 2) ? decorator(args[0], args[1]) : decorator;
}

Which is quite frankly horrendous. Additionally, I lose all valuable parameter intellisense against decorator F since now it has to be generalised.

There is a lot to be said to having decorators be easy and consistent to define and document. Lets not fool ourselves into thinking that everyone consuming our code will have the same savvy or understanding as us who are the creators of that code.

rbuckton commented 9 years ago

@Tharaxis I thought about this early on in the design for decorators, and I understand your specific points. As @mhegazy and @DavidSouther mention previously in this thread, this behavior is consistent with the standard behavior of functions in JavaScript.

You are correct that any attempt to create a function that can act as both a decorator and a decorator factory can be somewhat complex. In general though, it may be better to employ a best practice to always use decorator factories, and supply rules to a linter to ensure this practice is adopted.

Also, I have just submitted PR #3249 to address your earlier point regarding inconsistencies in the type checking for decorators.

pietschy commented 9 years ago

Hi @rbuckton,

I'm one of those not so savvy users of decorators mentioned by @Tharaxis...

I guess the question I would have is should decorators be consistent with functions? I.e. In the case of functions, returning this.f versus this.f() makes complete sense to me as sometime I want the value, sometimes I want the thing that produces the value.

In the case of decorators it doesn't seem quite as clear cut to me at the language feature level. I would like to argue that I just want to apply decorator @F, I don't really want to know whether it's been implemented as a factory or static method. That is unless I would loose some capability that decorators would otherwise have.

My apologies if the above is misinformed or ignorant, I'm relatively new the javascript world.

Thanks & cheers

Tharaxis commented 9 years ago

I also have to ask that question - does it make sense to maintain syntactic parity with with functions if (and only if) there is no real reason to do so other than "because it's defined using functions". I see Decorators as an entirely unrelated feature to functions which just happen to be defined using them.

Right now the fact that @F and @F() are not identical causes the following issues:

JsonFreeman commented 9 years ago

@Tharaxis I understand that the distinction between @F and @F() can be a high cognitive burden on consumers. However, it sounds like you are asking for the emitted code, and therefore the behavior, to change, so that the two behave equivalently. This would likely conflict with the ES7 proposal for decorators, since the two forms are very unlikely to be treated the same. It will not be good if this leads to a divergence.

Another point is that the confusion here is akin to the confusion between passing a function as a callback, and calling the function. If you had this

function callbackFactory() {
     return function(...args: any[]) { // do stuff };
}
function takesCallback(cb: (...args: any[]) => void) { }
takesCallback(callbackFactory());

A user may get confused and call takesCallback(callbackFactory) without invoking callbackFactory. This is really just like the confusion you pointed out with decorators, but if the language normalized these two forms to do the same thing, there would be a loss in expressivity. Sometimes the distinction is important, and needs to be maintained. Not sure if this commonly applies for decorators as well, but in theory it certainly could.

Tharaxis commented 9 years ago

@JsonFreeman I don't really get your argument though. You're saying that you can't make decorators make sense as their own entity because an ES7 proposal exists for decorators, but that's the thing, ES7 decorators are just that, a proposal, and as far as I'm aware, they're partly based on a combination of the decorator work described by both Google and yourselves, correct? Nothing has been decided on yet as to how they are to work, and it is likely that the proposal will go through many iterations prior to approval, so why not have them working in a consistent manner in TS and then (at least try) get the various parties involved in the proposal behind the idea, using experience gained in TS as a basis? This would not be the first time TS has implemented a feature only to have to refactor later because the ES6 spec called for slightly different behaviour. You'll always have that problem as long as you're tracking against a spec that does not yet exist and has not yet been agreed upon.

As for your example regarding functions, it appears yet again we're is conflating functions with decorators when they are in reality (and should be) completely different constructs. When I'm working with a function in JavaScript, I understand how functions are used and I understand the difference between a function reference and a function call (and when I might want to use one vs. the other) - and when I describe something as a function, there is no confusion as to what usages it has. Decorators are not functions and should not inherit behaviour of functions. Functions are merely the method upon which decorators are constructed. That's like saying classes are functions, which is not the case, functions are merely the way though which we describe classes prior to ES6 because there was no more obvious method available. Classes don't take on the properties of functions, you cannot call classes like functions, but you do declare them using functions prior to ES6.

Regardless, I feel that I've exhausted my points of argument on the issue. I'll just have to work around the choice. In my own code I'll always make use of factories for the sake of my own consistency. I still believe treating decorators literally as functions is likely to cause plenty frustration in the future, but that's just me.

EisenbergEffect commented 9 years ago

@mhegazy This is the same issue I brought up on the @jonathandturner decorator repo here: https://github.com/jonathandturner/decorators/issues/16 I do think this is something that should be thought about. As it stands, I expect this design choice to be the subject of many blog posts titled something similar too "ES7 Decorator Pitfalls".

JsonFreeman commented 9 years ago

@Tharaxis, I agree with your first point. I don't think an ES7 proposal should prevent us from designing something well, and our design is indeed one of the precursors to ES7 anyway.

As for the "decorators are not functions argument", if I'm not mistaken, we are not discussing decorators themselves, we are discussing decorator factories. And regardless of any distinction between decorators and functions, I think decorator factories are actually just functions. And it sounds like you are asking for the compiler to automatically generate a call to a decorator factory, which is an ordinary function. If that happened, there would be no way for the programmer to distinguish between the decorator factory and the decorator.

Also, the nice thing about treating decorators as functions is that a consumer can apply it as a decorator or by simply calling it as they see fit.

Tharaxis commented 9 years ago

@JsonFreeman the argument revolves around the current situation where there are two ways to both describe and invoke a decorator, one through a factory function, and then one as a "raw" function (which in the factory case is the result of a factory call), and the question was more about "shouldn't there just be one way, and have factories be that way".

What I am asking is yes, could we not just have the compiler turn @F into the equivalent of @F() call and have type checking require an argument list of 0...n parameters when using the unbracketed syntax. Perhaps you could elaborate on what is meant by "...there would be no way for the programmer to distinguish between the decorator factory and the decorator...", since I would think it's very easy to distinguish. The decorator is always the response from the factory, and the name of the factory is the decorator's name... not that difficult, or am I misunderstanding?

As for your last point about allowing the consumer to just apply the decorator, if it's well described that all decorators use factories, it's pretty easy to then invoke a decorator yourself, you just do <decorator name>(<argument list>)(target, name) compared to <decorator name>(target, name) as an example. Keep in mind that mandating the use of factories would mean the former example will always work, while not mandating it will result in the latter example sometimes not working, and is entirely dependent on how the decorator is implemented - a headache just waiting to happen.

I feel it necessary to point out, I have no problem with decorators using functions, my issue is however that having two ways to describe the same thing leads to consistency issues. Especially when it those two ways also mean your method of invocation has to differ. This is a disparity that hinders everything from refactoring to language consistency.

The refactoring problem I described a few posts back should already be more than enough of a reason why the current method needs to be inspected.

pietschy commented 9 years ago

Hi all, just a final 1.5 cents worth from a regular user along the lines of what @Tharaxis has said.

I'd be more than happy with current proposal if: a) the universe mandates it. b) decorators will loose important capabilities if things were different.

The later is of course a value judgment, to which the answer will depend to some degree on the type of developer you are (i.e. general user / expert user etc). I'm in the former category and am generally spread thinly across multiple languages and frameworks. So for me 'important capabilities' aren't going to include flexibility to write decorators 2 different ways. Examples of all the tradeoffs would be great if they exist.

Ideally it'd be great if @F and @F() could be consistent regardless of how decorators are implemented, but if not I'd vastly prefer to be constrained to using factories when writing decorators rather than having to avoid rakes in the grass whenever I'm using them.

Thanks & cheers

JsonFreeman commented 9 years ago

It seems like this request hinges on the idea that decorator factories are the canonical way to use decorators, but I don't see how this allows the user to define and use raw decorators. If I define a decorator F, and the application @F gets treated as @F(), then the result of calling F is used as the decorator, instead of F itself. Are you suggesting we give an error somewhere if someone tries to apply a raw decorator instead of a decorator factory?

This idea feels like it would reverse the natural compositionality of decorators and decorator factories. Decorators definitely feel like the primitive construct here, with decorator factories being a layer of abstraction built on top of decorators. They are just a pattern, nothing more. If instead the decorator factory became the canonical thing, the primitive, then people would define a bunch of decorator factories, that take no arguments and return a flat decorator. This would start to feel very silly, and it would reverse the natural intuition of what is considered basic and what is considered more complex.

One thing I am very wary of with decorators is an excess of magic. I personally get nervous when I don't understand what the code is doing, and if the compiler is secretly adding extra invocations that the programmer did not write, that feels to me like too much voodoo.

pietschy commented 9 years ago

Hi @JsonFreeman,

As I mentioned my preference would always be for the ugliness to be with the author of the decorator rather than the user. However, I agree that lots of no arg factories is a whole lot of ugly. Could a decorator be used to fix this? E.g.

// wraps rawDecoratorMethod in a no-arg factory method.
@Decorator
function rawDecoroatorMethod(target, name, descriptor) {...}
// looks like this one would be a no-op... so that feels not quite right unless there's other advantages.
@DecoratorFactory
function decoroatorFactoryMethod(someArg) {...}

Anyway, if such an approach could be found to make sense it has the advantage that the annotation documents the purpose of the function. i.e. it decorates.

Cheers

Tharaxis commented 9 years ago

Good gravy, decorators to help define decorators, a serious case of Typeception.

@JsonFreeman I'm not sure having zero-param functions are necessarily any more ugly than having a multitude of (target, name) functions in their stead. If anything at least the zero-param functions provide a level of specificity/explicitness that the other method lacks (since the parameters don't ever match the invocation in the latter case), and on top of that provides a single target to document instead of an unnecessary level of inconsistency. I also see a reluctance to go any one route due to a perceived "possible" complexity, but I would say in the light of the very real "obvious" complexity the current implementation imposes on the consumption side, one should err more on the side of the obvious than the possible.

The other option is to mandate that decorators which are called as @F are able to match either the pattern for ClassDecorator, MethodDecorator, PropertyDecorator or ParameterDecorator, OR a 0..n arg factory function that returns ClassDecorator, MethodDecorator, PropertyDecorator or ParameterDecorator. However I would feel that that implementation would cause other errors (what if you have two conflicting functions, which one would be considered best possible match?) and would just add undue complexity within the compiler. Simply converting @F calls to @F() would be the simpler solution and would solve the various problems stated.

JsonFreeman commented 9 years ago

Sorry, to clarify, I wasn't claiming that your solution is complex. I meant that automatically invoking the decorator before applying it is opaque. The inserted invocation is invisible to the user, and I feel like many users will not expect it.

I think the way it currently stands is not complex either. As you point out, it allows library authors to be vague or wishy-washy about whether their function is meant to be applied as a decorator. That I will grant you. But a good library will make it clear.

In the scheme you suggest, where you do the invocation automatically, what happens when you use an arbitrary decorator expression instead of just an identifier? Does that get invoked too?

Tharaxis commented 9 years ago

I'll agree, that unless documented otherwise the implicit invocation is likely to be surprising, but only to those that haven't used concepts such as C# attributes and the like.

Could you elaborate on what you mean by an arbitrary decorator expression?

pietschy commented 9 years ago

Good gravy, decorators to help define decorators, a serious case of Typeception.

Fair call. Serves me right for a poorly thought-out comment late on a weekend afternoon. Lesson learnt. Internet.undo().

My point being that if it turns out that consistent call site syntax mandates the use of factories then I'm more than happy with that. I'll no doubt write a decorator to remove the boiler plate. Again I'd prefer a little pain when writing the decorator over repeated pain using them (albeit potential pain at this point). Others will disagree.

Isn't there also the issue with API enhancement? A decorator created without a factory can't have optional parameters added at a later time, hence I predict @F and @F2() in my future.

I also don't see that you'd get this scenario when using f and f(), they're different use cases on the consumption side. In the decorator case I'm always applying/invoking the decorator on the target right then and there, and there's always a method invocation going on behind the scenes.

But the crux of this for me is the usability issue, when I'm applying a decorator I don't want to have to google to find out how the author implemented it - I'm interested in the behaviour, not the packaging.

Cheers

pietschy commented 9 years ago

Just a final note to summarise and then I'll keep quiet. In short, this is really just an interaction design issue for me. I'm really keen to see decorators designed from the outside in and not the other way around.

As a UI/UX guy I see this fairly often. I've worked with talented developers who have suggested UI solutions that would hurt users (one example was two save buttons in one case to solve a transaction complexity - again a smart guy and good human). I just think when you're knee deep in complex implementation details logic it can be hard to forget what you know and see it through the eyes of average user.

Now if as the average I'm just plain wrong, or if the complexity mandates the current design then all good, I'll just have to get my brain on and learn.

Cheers

JsonFreeman commented 9 years ago

If your decorator is not an identifier, but some other expression, would you automatically invoke it?For example, let's say it was a decorator. OR-ed with a fallback function expression. Something like:

@(F || function (target) { // default decorator behavior })
class C { }

Do you then automatically invoke this? That would give you the wrong result because you end up invoking the default decorator before it's applied:

(F || function (target) { // default decorator behavior })()

This does not seem right.

Tharaxis commented 9 years ago

@JsonFreeman, does the decorator syntax allow for arbitrary expressions like that? I'm not 100% certain allowing developers that much rope with which to hang themselves with is necessarily a good idea, but that's just me (purely because I don't see arbitrary expressions like that solving the reuse/boilerplate issue decorators aim to solve and instead just serve to make the code look ugly and even more difficult to follow).

That said, I suppose the only way it would work then would be if the arbitrary expression itself evaluates to the same factory syntax, so:

@(F || () => function(target) { /* default decorator behaviour */ })
class C { }

I agree it does result in you having to put a few more fiddly bits around your arbitrary decorator, but I think one has greater issues than the addition of () => if you're using arbitrary expressions as decorators.

NOTE: I obviously did not mean that lambda syntax would be the only way to declare it, but () => is slightly nicer than function() { return function(target) { /*...*/ } }.

qc00 commented 9 years ago

Really sorry to interrupt this decorator calling syntax debate, but could anyone officially clarify the order the decorator expressions are called? Especially, along the lines of decorator target type, the position of the target member in the original source and the order of the decorators on a single target.

JsonFreeman commented 9 years ago

@billccn The decorators are applied from bottom to top. So in the original source code if you had

class C {
    @F
    @G
    method() { }
}

This would first apply G to method, and then apply F to the result. Is this what you are asking?

qc00 commented 9 years ago

How about for this:

@A
class Clazz {
    @B
    prop = 1;

    @C
    method() {}

    @D
    get prop() {return 1;}

    @E
    method2() {}

    @F
    prop2 = 1;
}
mhegazy commented 9 years ago

they follow scope order, so all properties/methods first, in order of declaration, then the class one. i.e. B, C, D, E, F, A.

you can see the generated code here.