wycats / javascript-decorators

2.4k stars 127 forks source link

class decorator optional opts #23

Open cztomsik opened 8 years ago

cztomsik commented 8 years ago

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{
  ...
}
dead-claudia commented 8 years ago

I believe this is valid syntax:

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

That should solve your use case.

cztomsik commented 8 years ago

Yeah, I know but it feels weird.

dead-claudia commented 8 years ago

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 notifications@github.com 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 .

Aaike commented 8 years ago

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

dead-claudia commented 8 years ago

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.

dead-claudia commented 8 years ago

Although in practice, it is not quite that likely.

lukescott commented 8 years ago

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.

cztomsik commented 8 years ago

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

jeffijoe commented 8 years ago

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

dead-claudia commented 8 years ago

Shouldn't be, as far as I know.

On Wed, Nov 4, 2015, 05:42 Jeff Hansen notifications@github.com 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 .

jeffijoe commented 8 years ago

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.

cztomsik commented 8 years ago

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

dead-claudia commented 8 years ago

@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?

dead-claudia commented 8 years ago

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)
        }
    }
}
dead-claudia commented 8 years ago

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
})
cztomsik commented 8 years ago

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!

dead-claudia commented 8 years ago

@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.

cztomsik commented 8 years ago

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?

dead-claudia commented 8 years ago

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.

cztomsik commented 8 years ago

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

Download commented 8 years ago

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,,,

dead-claudia commented 8 years ago

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

cztomsik commented 8 years ago

@Download Yes, it's totally weird

dead-claudia commented 8 years ago

@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");
    })
});
silkentrance commented 8 years ago

Close, invalid/won't fix.

Download commented 8 years ago

@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.

dead-claudia commented 8 years ago

@Download

Is my last response at all an acceptable workaround?

@wycats Could you weigh in on this?

Download commented 8 years ago

@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.

dead-claudia commented 8 years ago

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 notifications@github.com 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 .

lukescott commented 8 years ago

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.

cztomsik commented 8 years ago

@Download yes! That was exactly my point - I just wasn't able to express enough clearly.

dead-claudia commented 8 years ago

@Download @cztomsik @lukescott You all are literally reinventing this utility I posted earlier in this issue (copy-pasted here for convenience):

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 : {})
        }
    }
}

const myDecorator = makeDecorator(function (target, name, descriptor) {
  // do stuff
  return descriptor
})

@myDecorator method() {} //-> myDecorator(prototype, "method", methodDesc)
@myDecorator("foo") method() {} //-> myDecorator("foo")(prototype, "method", methodDesc)
lukescott commented 8 years ago

@isiahmeadows And your utility breaks:

@myDecorator({}, "foo", {}) method() {}

It is far better to fix this in the spec and have the parser handle it. It has the proper context. If you have to use a utility at run time then @myDecorator({},"foo",{}) and @myDecorator cannot easily be distinguished. It's a hack to have to do this. The spec shouldn't encourage this at all.

What I'm proposing is arguments be passed after the first three as a rest spread. That way all you have to write is this:

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

So then it doesn't matter if you write @myDecorator or @myDecorator(...), the same function is used. No need for a utility to "fix things for you".

On top of that it would be nice if you could specify a different function for class methods vs classes. And give them a different signature. Hence Object.createDecorator({...}) or new Decorator({...}). It gives you even more clarity on what to do.

dead-claudia commented 8 years ago

@lukescott I did make that note in the original post. Also, the proposal is made such that if you would prefer, you can still call decorators as normal functions to the same effect. I really don't see much reason why it should have such a default.

By the way, in Python land, the behavior is identical to the current spec in that a decorator always has the same type. And that's where much of the inspiration for this proposal came from.

Over there, common practice is to only require parentheses if it accepts any arguments, and ignore them otherwise. Or if you really need such an overload, you can always do this:

def decorator(f, *args):
    if not isinstance(f, func):
        return lambda f: decorator(f, *args)

    # do whatever

In Python, objects' instance methods are always bound, unlike in JavaScript. Consider JavaScript a "consenting adults" language, just like what's been used to describe Python. If you choose to violate invariants of an API, it's your own responsibility for what happens, not that of the language or whoever wrote that API.

Also, :-1: for things like @readonly, @enumerable, etc., having to have a extra boilerplate than what needs to be. The closure is pointless IMHO.

// Current:
export function readonly(target, prop, desc) {
    desc.writable = false
}

export function enumerable(target, prop, desc) {
    desc.enumerable = true
}

// Proposed:
export function readonly() {
    return (target, prop, desc) => { desc.writable = false }
}

export function enumerable() {
    return (target, prop, desc) => { desc.enumerable = true }
}

Another frequent thing I do with decorators is use them as effectively wrappers for to DRYing code.

// Current:
function wrapper(func) {
    return (target, prop, desc) => {
        var old = desc.value
        desc.value = function (...args) {
            func(this, old.bind(this), ...args)
        }
        return desc
    }
}

const iterate = wrapper((ctx, old, ...args) => new Wrapper(old(...args)))

class Wrapper {
    constructor(iter) {
        this._ = iter
    }

    @iterate
    *map(func) {
        for (const entry of this._) {
            yield func(entry)
        }
    }

    @iterate
    *filter(func) {
        for (const entry of this._) if (func(entry)) {
            yield entry
        }
    }

    @iterate
    *takeWhile(func) {
        for (const entry of this._) {
            if (!func(entry)) break
            yield i
        }
    }
}

// With proposal:
function wrapper(func) {
    // Why does this have to be curried?
    return () => (target, prop, desc) => {
        var old = desc.value
        desc.value = function (...args) {
            func(this, old.bind(this), ...args)
        }
        return desc
    }
}

const iterate = wrapper((ctx, old, ...args) => new Wrapper(old(...args)))
silkentrance commented 8 years ago

@Download The issue might be ranking high but this still does not make the issue a valid one. And, considering #52 it seems that this is being used by the OP to whom I would refer to as a "Fönfrisur" to improve his personal google ranking. And considering it ranking high, you might want to make yourself a fool trying to investigate further along that line. Make my day... gets some snacks

silkentrance commented 8 years ago

@Download the context cannot be established as there is no way to distinguish a method decorator from a class decorator from a property decorator, especially more so with external libraries being used.

Or rather, it it is the other way around, the context is established by the parameters passed on to the decorator during runtime.

As such, you need to either use

@decorator
class foobar {}

or

@decoratorWithParameters(parameters)
class foobar {}

What is so problematic with adopting these semantics, where the rest of the 50%, i.e. 99.9999%, did already embrace it and are happy with it?

lukescott commented 8 years ago

@isiahmeadows Just to make sure I'm being clear, I'm proposing this:

export function readonly(target, prop, desc, ...args) {
    desc.writable = false
}

export function enumerable(target, prop, desc, ...args) {
    desc.enumerable = true
}

@readonly and @readonly() would use the same signature above. Is that not reasonable?

@silkentrance You're welcome to share your opinion, but your tone is not appreciated.

And btw, the parser has context where the decorator is being used. For example:

// currently in module context
@myDecorator // decorator
class Foo { // class declaration in which decorator must be applied - Foo becomes an argument of myDecorator - parser knows this is a class decorator
// inside class. parser knows this so it can parse methods and properties.
@myDecorator // decorator
  method() { // method descriptor is passed to myDecorator as an argument

  }
}
silkentrance commented 8 years ago

@lukescott look, in order for this to work, the babel compiler needs to have indepth knowledge of the decorator that you are using, whether it takes additional parameters and thus represents a decorator factory. And in order for this to work it must also parse and be able to statically analyze the source of that decorator.

Either way, this is not possible. Read me, not possible.

lukescott commented 8 years ago

@silkentrance This is spec discussion. Not babel discussion. And I really don't follow your logic. Decorators are plain functions (right now). How can they be statically analyzed?

@kittens can you chime in on this? Thanks!

silkentrance commented 8 years ago

@lukescott you are stalling. the spec should have been well on its way for months now. Besides that, same source can be statically analyzed as it is well the case with existing babel transformers. External sources cannot, and that is the point I am making.

Considering that babel is a reference implementation of this spec.

lukescott commented 8 years ago

@silkentrance:

You are making no sense to me, sorry.

This isn't a spec. It's a proposal. Stage 1. One of the purposes of a proposal is to "identify potential challenges". Language features take years. And they aren't going to be rushed in willy nilly because you think they should. Just because babel gives you the ability to use these features now does not make their current implementation final or a guaranteed addition.

See https://tc39.github.io/process-document/

And sorry, but I must have missed you as a babel contributor. Perhaps you are submitting code under a different name? I mean, you must be because you speak with such authority.

I have contributed to the babel project, although personally very little. If something is "impossible" in babel, let Sebastian or one of the major contributors say so.

silkentrance commented 8 years ago

@lukescott Just get over it. Besides of that I am a user of the feature and I want it to be exactly the way it was originally proposed to be. Of course I also want to have free function decorators, but that is a different story.

Nonetheless, the feature you are asking for is impossible to do and also disrespects the achievements made by other languages such as Python and Java in that regard, which of course, this spec proposal is based upon.

I´d say we should stick to well established patterns here and not enforce an anti-pattern, similar to the backslash in PHP namespaces.

loganfsmyth commented 8 years ago

To clarify, this proposal, and the one in #52 are essentially to have a Babel-style psuedocode transform that says

  1. For each decorator "dec"
    1. if "dec" is a simple identifier "foo"
      1. replace "foo" with a call to "foo": "foo()"

assume I follow it correctly. You can potentially view it as the same type of thing where var inst = new Constructor; and var inst = new Constructor; behave the same, even though one does not have parens.

@silkentrance Nothing about this is impossible. I agree that it is confusing and I don't think it's a good idea.

Just get over it. Besides of that I am a user of the feature and I want it to be exactly the way it was originally proposed to be.

It is a discussion, people are allowed to disagree. Nothing about your opinion vetos other people's opinions. No amount of stating that your opinion is more important is going to make people discard their opinions.

loganfsmyth commented 8 years ago

@lukescott I'll pull in and expand on my example from #52 since I'm curious for thoughts. My concern with this proposal is that it trades one case of unexpected behavior for another. Automatically converting an identifier reference to a call in the context of a decorator would help developer experience in the specific case you've mentioned, but it would hinder it in other cases. Take for example

function decorator(){
    return function(target, prop, desc){};
}

class Example {
    @decorator()
    method1(){}

    @decorator()
    method3(){}

    @decorator()
    method3(){}
}

With your proposal, this works with or without the parens on each call. Cool.

Say you go to refactor this code at some point, because you realize you're calling the same decorator factory function many times. In the existing world, and I'd argue anywhere else in JS, you can safely extract the decorator() call and assign it to a variable, then use that variable in its place, e.g.

function decorator(){
    return function(target, prop, desc){};
}

var dec = decorator();

class Example {
    @dec
    method1(){}

    @dec
    method3(){}

    @dec
    method3(){}
}

expect now with your proposal, the above will be incorrect, because the dec reference will automatically be considered a call, and thus treated as @dec(), which would break the code, and as a developer you will have no recourse except to manually do something like

function decorator(){
    return function(target, prop, desc){};
}

var decFunction = decorator();
var dec = () => decFunction;

class Example {
    @dec
    method1(){}

    @dec
    method3(){}

    @dec
    method3(){}
}
sebmck commented 8 years ago

@silkentrance Your tone is extremely disrespectful and I'd advise you to change your tone if you want anyone to take you seriously. Babel is not a reference implementation, this proposal has undergone extensive changes and is still in development, Babel 6 does not ship with any decorators implementation because of this. None of the activity in this repository is stalling this proposal. The willingness and organisation of the proposal champions is. Please don't speak out of turn and let's have some respectful and insightful dialogue. All of your messages have been rife with personal abuse and strong negative language.

silkentrance commented 8 years ago

@kittens to be frank, yes it is and it needs to be, frankly because the feature of always having to decorate using @decorate()(withparamters)(with additionalparamters)(with extra parameters) is just beyond my mind. Namely that first empty set of brackets, which is uttlerly redundant,

And everyone in favor of such a proposal is definitely insulting mine and the intelligence of the other 99.999% developer´s out there, while the rest of the 50%, according to #52, which is you, seems to be in favor of it.

So let us make it draw. Reason wins. 99.999% against 50%, or 99 to 1.

loganfsmyth commented 8 years ago

If people having opinions that differ from your own is something you consider an insult, you're going to have a bad time in life. People are allowed to disagree, if you choose to take that as an insult, we can't stop you, but please don't assume that that is how everyone else sees it. I don't agree with this proposal, but I am also not choosing to attack the intelligence of those that have proposed it.

You can assert with full confidence that you are correct, the rest of us can do the same, but that just leaves us at a deadlock. By refusing to participate in the discussion and instead falling back on declarations of absolutes, you do nothing to move this conversation forward, and impede us from coming to any kind of conclusion.

silkentrance commented 8 years ago

@loganfsmyth thanks but no thanks.

I am trying to move this conversation forward as in bydesign. So that we can all move along and consider this issue a be a non issue that was already fixed by design?

And "bydesign" it is. Unless decorators need to implement a specific interface that is not yet a part of the specification. And even if they did, implementations of the spec would still have a hard time figuring out what is which, considering import decorator from ....

As such the whole shebang here is invalid and not really worth of any of us` time,

Just close and be happy with it, because you did the right thing.

alexbyk commented 8 years ago

I moved here from https://github.com/wycats/javascript-decorators/issues/52 . I decided to merge this issues because they are similar.

Synopsys: 1) Right now it's impossible to write a smart universal decorators that work both ways Injectable('some', 'opts') class MyClass {} and Injectable class MyClass {}. And because of that it's very boring to write () every time instead of simple just @log attr, @Inj MyClass {} and so on 2) The most of the developers (95.52%) don't know 1). They try to write smart decorator with lot of a code. That is wrong. And as a result we have a lot of bugs (heisenbugs, security issues and so on) I made an anti-guide to describe why here https://github.com/alexbyk/decorators-flaw

That's because in fact right now we have 2 types of decorators. (1) DecoratorWorker and (2) DecoratorFactory that returns a (1) DecoratorWorker and it's not obvious when to use 1 and when 2. Using 1) will lead to either refactoring in the future, or to errors.

Proposal. So my +1 is for: 1) Stop supporting decorators of the 1) type. with @decorator THINS, decorator should be a function that returns a function: (2) DecoratorFactory 2) Make @decorator THING the same as @decorator() THING - because writing () everytime is boring

Examples

This and only this decorator is right try it

function log(prefix){
    return (Class) => {
      console.log((prefix || 'LOG: ') + Class.name);
    };
}

@log()
class Foo {
    method(){}
}

This is wrong (but works in the current implementation)

function log(Class){
      console.log(Class.name);
}

Below are examples. It's impossible* to implement it right now. If you thing it's possible, you are wrong and should read this anti-guide to understand why https://github.com/alexbyk/decorators-flaw . Because many developers, event maintainers of top rated libraries do the same mistakes. Right now there are a lot of buggy ugly code with decorators in the github. And that's because of ES7 decorators design flaws.

But my proposal makes it possible

@Injectable class Foo {};
@Injectable({opt1: 1}) class Foo {};
class Foo {
 @inj one;
 @inj two;
 @inj('THREE') three;
}

And once again. If you think you can implement 2 examples above right now, you're wrong, you can't because of current decorators flaws, read how John tried and what happened to him

cztomsik commented 8 years ago

@Download expressed it the best - I would expect @dec and @dec() to work the same - how about create a poll for this so we can see how many people agree/disagree with us?

Also, I don't think Python is a valid argument - or at least not more valid than let's say Java.

From what I see the only reason to not fix the spec is that there is a lot of code already using it in a current way. That is unfortunate, yes - you've used something which is not yet standardized and so you would have to rewrite some of your code but it's IMHO better than extending language with another "bad part"