wycats / javascript-decorators

2.4k stars 127 forks source link

"this" inside the decorator #72

Open mjesun opened 8 years ago

mjesun commented 8 years ago

What should be the value of "this" inside the decorator? For example if you do something like:

@Decorators.decorate
class A {}

When being inside decorate, the value of this should be Decorators, right?

I'm asking that because I haven't been able to find this inside the proposed specification, and I think it's nice to have it defined. Also, if that's the case, the current plugin for babel (the babel-plugin-transform-decorators-legacy) should be changed to support it. Right now the compilation is done as:

(_dec = Decorators.decorate, _dec(_class = function () { ... }))

Which would be incorrect (this is undefined in strict mode, or the global object in loose mode).

silkentrance commented 8 years ago

@mjesun Actually, the this is not defined. The decorator is considered to be a free function and I believe this to be the correct approach.

That way, you can define more complex decorators where you can provide your own this.

mjesun commented 8 years ago

Maybe I'm missing something; but I guess you can't provide your own this, unless you bind your method beforehand; since you do not have any control on how the method is invoked. It's also simpler for me to expect that a decorator defined as @a.b respects the way this has been working since the very beginning in JS.

otakustay commented 8 years ago

I'm on the opposite side, the declaration and invocation does not happen at the same time, when you write code like:

@a.b

is just a declaration which just like:

var hereIsADecorator = a.b;

So when decorator is invoked, it is:

hereIsADecorator();

which should not provide any this value.

mjesun commented 8 years ago

I can get that point; however I don't see the benefit of it. If this was used for something else, then I think I'd be OK. However, this is not used for anything, so why just not keeping the context value?

I assume you want to keep it undefined because you see the declaration and the invocation at different point in times, but I see that there are nicer advantages in keeping the context and considering that the declaration and invocation happens all at once (e.g. being able to use static class methods as mixins).

Still, the proposal is yours, so if I can't convince you I think we can close the ticket :smiley:

otakustay commented 8 years ago

I'm not the author but I believe assuming a this value in decorator impolementation could be a dangerous thing since the author is not arware how this decorator would being used

For example if we implement a decorator like this:

// decorators.js
export default {
    name: 'whatever',

    foo(target, key, descriptor) {
        target._hasDecorator = this.name;
    },

    bar() {
        // ...
    },

    // ...
};

but is used like this:

import decorators from 'decorators';

let {foo, bar} = decorators;

export default class OhMyGod {
    @foo
    @bar
    work() {}
}

The this value is lost

If this is required, it is better to bind it on implementation:

const context = {name: 'whatever'};
function foo() {
}
export default {
     foo: foo.bind(context)
};

This is much safer