jayphelps / core-decorators

Library of stage-0 JavaScript decorators (aka ES2016/ES7 decorators but not accurate) inspired by languages that come with built-ins like @​override, @​deprecate, @​autobind, @​mixin and more. Popular with React/Angular, but is framework agnostic.
MIT License
4.52k stars 263 forks source link

chaining multiple @decorate's fails in 0.12.1 #73

Closed TrejGun closed 8 years ago

TrejGun commented 8 years ago

in 0.12.0 this code works

"use strict";

import {decorate} from "core-decorators";

function decorator(fn) {
    return function inner(...args) {
        console.log("fn", fn);
        return fn.bind(this)(...args);
    };
}

const obj = new class {

    @decorate(decorator)
    @decorate(decorator)
    test(...args) {
        console.log(...args);
    }

};

obj.test();

in 0.12.1 it throws

test.js:43
                return fn.bind(this).apply(undefined, arguments);
                         ^

TypeError: Cannot read property 'bind' of undefined
    at _class.inner (test.js:8:10)
    at Object.<anonymous> (test.js:22:5)
    at Module._compile (module.js:413:34)
jayphelps commented 8 years ago

This is a toughy...

The issue is that we switched to lazily applying the decorator using a getter so that we can apply it to the instance instead of the prototype which resolved another bug. The test suite missed it because I wasn't testing chaining multiple @decorate calls. I'm experimenting with some solutions.

jayphelps commented 8 years ago

@TrejGun I believe I've fixed it with #74, gonna let the code stew for a day. Feel free to try if you can.

You can try it out either by building core-decorators yourself or you can download this zip and extract the contents into your node_modules/core-decorators/* core-decorators.zip

TrejGun commented 8 years ago

LGTM, thanks!

TrejGun commented 8 years ago

will this work with something like

    @autobind
    @decorate(decorator)
    test(...args) {
        console.log(...args);
    }

or

    @decorate(decorator)
    @autobind
    test(...args) {
        console.log(...args);
    }

?

jayphelps commented 8 years ago

@TrejGun not at the moment (one breaks the other invokes the method pre-emptively internally so be careful)

This whole thing has been an awful juggling between different requests (e.g. #65), so I'm considering reverting back to the old way of decorating the prototype instead and then introducing an option or a new decorator that will then opt-into decorating the instance instead.