javascript-decorators

javascript-decorators copied to clipboard

Is it really necessary to make difference between regular class decorators and those accepting arguments?

Because it would be great if we didn't have to test presence of arguments, how would you implement @dec with optional arguments?

It feels inconsistent - I'd rather have to return function all the time instead of doing nasty isClass checks

@dec({...})
class SomeClass{
  ...
}

@dec
class AnotherClass{
  ...
}

I believe this is valid syntax:

@dec()
class AnotherClass{
  // ...
}

That should solve your use case.

Yeah, I know but it feels weird.

True. I was thinking that as I typed that reply. And as for the checking, I don't think it's worth adding that boilerplate to the proposal, though. Especially considering the recent disdain on es-discuss over proposed new language features that are better off as macros.

On Sat, Jun 20, 2015, 06:55 Kamil Tomšík [email protected] wrote:

Yeah, I know but it feels weird.

— Reply to this email directly or view it on GitHub https://github.com/wycats/javascript-decorators/issues/23#issuecomment-113745865 .

i have been struggling with the same thing. I wanted an easy way to create decorators that could be used with or without the parenthesis. I ended up creating some utility functions to help with that, it's probably not the best way of doing it , but it works : https://github.com/gooy/js-deco

If you want a proper, complete checker, you'd have to write a helper function for it, and it could still be theoretically fooled if it's given something that looks enough like a duck.

Although in practice, it is not quite that likely.

Is there any reason to not require the outer function to always return a decorator function as the OP suggests? It would make things far simpler.

@lukescott yeah, that was my point - but I'm afraid it's too late as there is a lot of libs already using it.

@lukescott @cztomsik I think always wrapping in a function makes sense, to avoid confusion. Existing libs shouldn't be hard to fix?

Shouldn't be, as far as I know.

On Wed, Nov 4, 2015, 05:42 Jeff Hansen [email protected] wrote:

@lukescott https://github.com/lukescott @cztomsik https://github.com/cztomsik I think always wrapping in a function makes sense, to avoid confusion. Existing libs shouldn't be hard to fix?

— Reply to this email directly or view it on GitHub https://github.com/wycats/javascript-decorators/issues/23#issuecomment-153680654 .

I have 2 production app's that are heavily powered by custom decorators, so I know for a fact that it won't be difficult.

Do you really want to see warnings like these all over the internet?

@cztomsik Either they didn't think of this, or they don't trust this:

function InjectableImpl(target, prop, desc, opts = {}) {
    // ...
}

function Injectable(opts, prop, desc) {
    // You can check `opts` and/or `prop` as well, if you want extra insurance.
    if (arguments.length === 3 && typeof desc === "object") {
        return InjectableImpl(opts, prop, desc)
    } else {
        return (target, prop, desc) => InjectableImpl(target, prop, desc, opts)
    }
}

It's possible to overload a function and just check the types to make sure it's not being used as/like a decorator. Should this be able to fix the other half of the issue?

That's also possible in TypeScript as well. You just need to overload the function type signature.

function InjectableImpl(
    target: any,
    prop: string,
    desc: PropertyDescriptor,
    opts: Options = {}
) {
    // ...
}

interface InjectableDecorator {
    (target: any, prop: string, desc: PropertyDescriptor): any;
}

function Injectable(opts: Options): InjectableDecorator;
function Injectable(opts: any, prop: string, desc: PropertyDescriptor): any;

function Injectable(opts: any, prop: string, desc: PropertyDescriptor) {
    // You can check `opts` and/or `prop` as well, if you want extra insurance.
    if (arguments.length === 3 && typeof desc === "object") {
        return InjectableImpl(opts, prop, desc)
    } else {
        return (target: any, prop: string, desc: PropertyDescriptor) => {
            InjectableImpl(target, prop, desc, <Options> opts)
        }
    }
}

So, it should be possible to optionally take arguments, if you're willing to accept a little bit of boilerplate.

For a general solution assuming a single options object is taken:

function makeDecorator(callback) {
    return function (opts, _, desc) {
        // You can check `opts` and/or `prop` as well, if you want extra
        // insurance.
        if (arguments.length === 3 && typeof desc === "object") {
            return callback({}).apply(this, arguments)
        } else {
            return callback(opts !== undefined ? opts : {})
        }
    }
}

// Example usage:
var Injectable = makeDecorator(function (opts) {
    return function (target, prop, desc) {
        // do stuff
    }
})

Or if you'd prefer just a single function:

function makeDecorator(callback) {
    return function (opts, prop, desc) {
        // You can check `opts` and/or `prop` as well, if you want extra insurance.
        if (arguments.length === 3 && typeof desc === "object") {
            return callback({}, opts, prop, desc)
        } else {
            if (opts === undefined) opts = {}
            return function (target, prop, desc) {
                return callback(opts, target, prop, desc)
            }
        }
    }
}

// Example usage:
var Injectable = makeDecorator(function (opts, target, prop, desc) {
    // do stuff
})

Why do we have to do this in the first place?

if (arguments.length === 3 && typeof desc === "object") {
    return InjectableImpl(opts, prop, desc)
} else {
    return (target, prop, desc) => InjectableImpl(target, prop, desc, opts)
}

It's cryptic!

@cztomsik All I did was check the arguments and a simple sanity check to ensure the last argument at least somewhat looks like a descriptor. It's just an overloaded function, where it either applies the decorator with no options, or binds the options and returns a decorator. Nothing cryptic there, unless you're referencing the ES6 arrow function. It's just 5 lines of code.

And like I also commented, there's still a way to conceal all that behind a function. And I ended up using another similar technique to ensure both of these worked as anticipated:

var f = Param(Foo)(function (foo) {
  return foo + 1
})

class Foo {
  @Return(Types.Number)
  bar() { return 1 }
}

Believe it or not, that overload is slightly more complicated than this.

Sorry, my point was that spec itself leads to cryptic code - in its current state it's hard to explain to coworkers.

If there was no if/else, it would have been easier. What's an actual usage of paramless version - in which case it is better than regular (target, prop, desc) => ... decorator?

And if there is any - is it really worth of bugs it may cause - or extra safety code you will have to write?

And in general, most decorator-based libraries and frameworks coming out now either always require a call like Angular's Injectable(), or always require a call unless the decorator never takes arguments, like several in core-decorators.

Yeah, and that's why they have such big alert in docs ;-)

Sorry for chiming in here but this issue ranks high in Google on my searches for information about using braces or not with decorators...

I wrote a small test case, but the results are not what I am expecting. From reading this issue I understand this may actually be correct though:

describe('decorator', ()=>{
    function myDecorator() {
        return (target) => {target.decorated = true; return target;}
    }

    it('works when called with braces', ()=>{
        @myDecorator()
        class Test {}
        expect(Test.decorated).to.equal(true);
    })

    it('works when called without braces', ()=>{
        @myDecorator
        class Test {}
        expect(Test.decorated).to.equal(true);
    })
});

Output:

decorator
  √  works when called with braces
  ×  works when called without braces
      AssertionError: expected undefined to equal true

I would have expected the same results in both cases... If I add logging to the myDecorator function, I see that it is called twice... So probably this is exactly what you guys are talking about in this issue?? I have to look at arguments.length somehow?

Can anyone here maybe refer me to the section in the spec that describes this?

And to add my 2 cents... I find the current behavior very unexpected so maybe other devs will feel the same... It may be too confusing for people. I think it would be best if decorators worked the same way no matter what they are decorating,,,

@Download My workaround is admittedly a hack, but it's a conceptually simple one that doesn't really require much code.

@Download Yes, it's totally weird

@Download Also, your example isn't correct. And it shouldn't work. myDecorator returns a function, and ignores all its arguments. So I would expect the first example to return the arrow function. Here's what I would expect your example to be, if it were to be passing:

describe('decorator', () => {
    function myDecorator() {
        return (target) => {target.decorated = true; return target;}
    }

    it('works when called with braces', () => {
        @myDecorator()
        class Test {}
        // Test is a class.
        expect(Reflect.isConstructor(Test)).to.equal(true);
        expect(Reflect.isCallable(Test)).to.equal(false);
        expect(Test.decorated).to.equal(true);
    })

    it('works when called without braces', () => {
        @myDecorator
        class Test {}
        // Test is an arrow function.
        expect(Reflect.isConstructor(Test)).to.equal(false);
        expect(Reflect.isCallable(Test)).to.equal(true);
        expect(Test).to.not.have.property("decorated");
    })
});

Close, invalid/won't fix.

@silkentrance I hear reasonable concerns voiced here on a proposal that is still WIP...This issue ranks high in Google when searching for info related to this... So please don't close just yet... At least these concerns should be adressed in the specs somehow.

@Download

Is my last response at all an acceptable workaround?

@wycats Could you weigh in on this?

@isiahmeadows Your response is more or less describing how it works now, however I was describing how I would expect it to work.

Based on my experience with other languages (notably Java which has annotations with a very similar syntax) and on 'common sense', I would expect @myDecorator (without braces) to be functionally equivalent to @myDecorator() (with braces). I expect the transpiler to notice the missing braces and 'insert them' as it were. If it did that, both cases would yield an arrow function, which would in both cases be called with the class as an argument.

I am probably missing something very big, but at this moment I see no advantages to not doing it this way... only a lot of confusion... But there probably is a very good reason not to do it this way... I just don't see it.

My thing was that I feel that workaround should be sufficient for most use cases. I don't see the need to complicate the language further just for this.

On Tue, Mar 1, 2016, 09:26 Stijn de Witt [email protected] wrote:

@isiahmeadows https://github.com/isiahmeadows Your response is more or less describing how it works now, however I was describing how I would expect it to work.

Based on my experience with other languages (notably Java which has annotations with a very similar syntax) and on 'common sense', I would expect @myDecorator (without braces) to be functionally equivalent to @myDecorator() (with braces). I expect the transpiler to notice the missing braces and 'insert them' as it were. If it did that, both cases would yield an arrow function, which would in both cases be called with the class as an argument.

I am probably missing something very big, but at this moment I see no advantages to not doing it this way... only a lot of confusion... But there probably is a very good reason not to do it this way... I just don't see it.

— Reply to this email directly or view it on GitHub https://github.com/wycats/javascript-decorators/issues/23#issuecomment-190741201 .

Complicate the language? Checking arguments.length is complex. Expecting @myDecorator and @myDecorator() to "do the same thing" seems reasonable to me.

Why is the decorator syntax like this?

function myDecorator() {
  if (arguments.length === 3) {
    let [target, name, descriptor] = arguments;
    return actualMyDecorator(target, name, descriptor);
  } else {
    return actualMyDecorator;
  }
}
function actualMyDecorator(target, name, descriptor) {
  // do stuff
  return descriptor;
}
@myDecorator method() {} //-> myDecorator(prototype, "method", methodDesc)
@myDecorator("foo") method() {} //-> myDecorator("foo")(prototype, "method", methodDesc)

When it could be this?

function myDecorator(target, name, descriptor, ...args) {
  // do stuff
  return descriptor;
}

@myDecorator method() {} //-> myDecorator(prototype, "method", methodDesc)
@myDecorator("foo") method() {} //-> myDecorator(prototype, "method", methodDesc, "foo")

Checking length and types isn't fool proof either. What if you expected 3 arguments? What if the first argument was an object? Seems very fragile to me.

And to add to this, what if a decorator is used on a method or a class? How do you know which one? More type checking? I added issue #40 for that. The general idea is you do something like this:

var myDecorator = Object.createDecorator({
  classMethod(target, name, descriptor, ...args) {
    // do stuff
    return descriptor;
  }
});

The above would only support myDecorator on a class method. If you wanted to also use it on a class:

// Alternative: new Decorator({...})
var myDecorator = Object.createDecorator({
  class(Class, ...args) {
    // do stuff
    return Class;
  },
  classMethod(target, name, descriptor, ...args) {
    // do stuff
    return descriptor;
  }
});

Notice how each function signature matches the context of where the decorator is being used.

Also, having something like Object.createDecorator makes a decorator more intentional. It prevents a plain JavaScript function, object, or plain value from being used as one.