Force decorators to be higher-order functions even without parentheses

[alexbyk]

Right now there are 2 forms of @decorators: (1)decoratorFunction and (2)decoratorFactory, which produces a (1)decoratorFunction. The difference for end users is only parentheses. If there are parentheses, decorator is in the 2nd form, if there are no parentheses, it's in the 1st form

The problem is: while we have 2 possible variants, it's not obvious which one to choose, and that leads to mistakes My proposal is to forbid the (1)decoratorFunction form and consider a decorator as (2)decoratorFactory, even without parentheses If my proposal is accepted, there will be no forgot parentheses bug. Also, because parentheses without arguments will be always optional, the syntax will be much clearer in the most of the cases

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

the same, parentheses without args are optional

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

desugaring:

var Foo = (function () {
  class Foo {
  }

  Foo = F("color")(Foo = G()(Foo) || Foo) || Foo;
  return Foo;
})();

and G should be written this way:

function G(optional) {
  return function G(Class) {
    Class.prototype.g = optional ? optional : 'hello';
  };
}

P.S. Writing universal decorator, that examines arguments in runtime and behaves either (1) or (2) is impossible. See proofs below. Or, if you don't understand why, here is my step-by-step explanation with examples: https://github.com/alexbyk/decorators-flaw . Also please read it before pointing me to the SomeAwesomeLibrary because in the most of the cases all libraries that provide "universal/smart" decorators syntax have the same bugs no matter how popular they are. So if you library provide @withSomething class Foo {} syntax, it 100% has a potential bug. The same thing about attribute decorators

[PinkaminaDianePie]

Its currently worked, if you are writing decorator this way:

let decorator = function(...args){
  let decoratorArgs = [];
  let decorateHandler = function(target){
    console.log(target.name , decoratorArgs)
  }
  if (typeof args[0] === 'function'){
    decorateHandler(...args)
  }
  decoratorArgs = args;
  return decorateHandler;
}

@decorator
class C{

}

@decorator()
class C2{

}

@decorator('foo')
class C3{

}

it outputs:

C []
C2 []
C3 ["foo"]

So you just need to check, if first parameter is your class - if so, decorator was called without brackets and you can apply your logic in decorateHandler, otherwise it was called with brackets - with some parameters ot without, so we return decorateHandler and stores arguments in clojure. So i think the problem is not with this proposal, but with usage by developers.

[alexbyk]

@PinkaminaDianePie Try you decorator with this case

@decorator(function(){})
class C3{}

So there's no way to distinguish this case when args.length === 1 && typeof args[0] === 'function' I was very surprised when I've faced this case and tried to solve it the similar way.

[PinkaminaDianePie]

@alexbyk how about

@decorator(function(){})
class C3 extends YourBaseFrameworkClass{}

and check it in decorator with isPrototypeOf?

[alexbyk]

@PinkaminaDianePie I'm proposing to solve problems with decorators, not to invent new ones. For example, I use this variant with decorators because it's simple and clear. If it make my code more complex (like requirement to extend some other class), I'll better avoid it.

But thanks for your feedback

[mastilver]

Have a look there: https://github.com/mastilver/decorator-router/blob/master/index.js#L35

Really similar to @PinkaminaDianePie approach Without the function issue

[alexbyk]

@mastilver Could you please provide an example I can copy-paste-run like @PinkaminaDianePie did? Because I described a glitch in the current design of class decorators (and provided an edge case when it's impossible to distinguish a single argument-function from the wrapped class), which can't be resolved. As far as I can see, you gave an example that isn't relevant to this topic because isn't a class decorator at all. (I made 2 words bold to show the difference)

I'm asking for an example (copy-paste-run, which should solve this edge-case too) for a simple reason: if I'm right, nobody can provide such example, and that's my point - it's impossible. And, obviously, if someone can provide a solution, I'm wrong

[mastilver]

Sorry, my bad. You are right, there is no way you can tell if target is a function you pass to the decorator or the class you are decorating

[loganfsmyth]

I feel like the whole phasing of this is confusing. Decorators are never higher-order functions. If you're doing decorator() then you've named your function wrong because decorator isn't a decorator at all, it's createDecorator(). A decorator is a function that takes standard spec params, if you have a function that returns a decorator, it's not a decorator, it's a factory function.

If you want to combine those two into a function that can do both, it seems like you're just asking for trouble to begin with. If something has two different semantics, pretending it can be used the same way is just asking for trouble.

[alexbyk]

@loganfsmyth I don't try to combine anything. I'm assuming many people will try to do that. I know how decorators differ from factories, you know that too...That's a theory

And here is how things work in practice: 1) Someone makes TheAwesomeLib which exports @superClassDecorator 2) Then he decides to add more features and accept optional arguments, and rewrites it to @superClassDecorator(a). To provide backward compability, he decides to make it universal (see how other participants tried to do that above) 3) Surprise. Universal version doesn't work as expected, but that's too late.

So my point isn't "it's impossible to make universal version of decorator+factory", it sound more like "Everybody thinks it's possible... and probably tries to make it universal... and gets a surprise because it isn't". So my proposal was very simple: fix decorators syntax to avoid such problems in the future. Why? Because I expect many problems with the current syntax in the future and many "Cooking decorators: Pitfalls" blog posts. I faced this problem 1 or 2 times by myself.

[alexbyk]

Ok. Here is a changed version of desugaring. G() instead of G

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

var Foo = (function () {
  function Foo() {
  }

  Foo = F("color")(Foo = G()(Foo) || Foo) || Foo;
  return Foo;
})();

With this nobody will try to make a buggy universal version and problem won't happen

[loganfsmyth]

Wouldn't the proper way to handle this then be to export the factory under a new file or property, e.g. @decorator.create(), or to bump the major version and change the API?

My primary concern is that this is a total departure from how js generally behaves. Nowhere else in js does a property or identifier access magically become a call.

[alexbyk]

@loganfsmyth About your concerns: right now neither @something nor @something() is a valid js code at all, so all @decorstors stuff is a magic.

You're right about the proper way. But problem is other people will not do it the right way, because it's a trap. This topic is a proof: look at the top, other 2 participans (I have no doubt they are smart and experienced developers) tried to convince me that there is a solution. And I'm sure there are many other people, who thinks the same way (I felt into that trap once by myself).

[loganfsmyth]

To clarify my "how js generally behaves" point, the issue with this is that it means

@decorator()
class Example {}

// @decorator()(class Example {});

and

var dec = decorator();

@dec
class Example {}

// @dec()(class Example {});

would behave differently. There is no other case in JS that comes to mind where you can move an expression into an assignment, and then use the variable in its place, and have it behave differently.

I get that you could certainly define it this way, but then you are trading potential confusion for decorator developers, with potential confusion for decorator users, and users are the far more common case.

[alexbyk]

@loganfsmyth Your explanation, surely, makes sense too. But I think it's all syntax/desugaring/definition. My proposal makes () optional. I can give you an example when () syntax is optional too: bar instanceof Bar and bar instanceof(Bar), not very good example, but only to show that this syntax variation exists and everyone is ok with it.

If we don't define decorator part in @decorator as a function variable, my variant will make sense without confusing too

[alexbyk]

I have a suggestion. We have already seen enough pros and cons. How about a poll by :+1: :-1: ? Only to see does anybody care :) Mine is :+1:

[alexbyk]

The same problem about "properties" decorator:

class Person {
@Inject  foo=3;
@Inject(FN, 'arg') bar = 4;
}

It's very easy to forget parentheses and there are cases when it's impossible to distinguish syntax and make a smart decorator.

I also found a proof that problem exists in Angular2 docs , commit is here

Don't forget the parentheses! Neglecting them leads to an error that's difficult to diagnose.

Typescript inherited this problem.

[silkentrance]

@alexbyk I think that you are missing the point here.

Decorators are by default functions, yes, but calling them is left to the framework, in that case the language. In other cases you have decorators that take additional parameters, which the language cannot address for. Here, you have to call the decorator (factory) first with the set of parameters that customize the outcome of the decorated method or whatever you are decorating it with, e.g.

function decoratorFactory(param1, param2) {
   return function decorator(target, attr, descriptor) {
   };
}

function decorator(target, attr, descriptor) {
   ...
}

@decoratorFactory('1', 2)
class Foo {
     @decoratorFactory('3', 4)
     bar() {}

     @decorator
     get foobar() {}
}

Why would you want to deprive both the developers and users of decorators from such a nice feature?

[silkentrance]

Please close as won't fix.

[loganfsmyth]

@silkentrance For the record, I don't disagree with your logic here or in the other replies you've just posted in this repo.

That said, the tone of your comments like this are massively unnecessarily dismissive. These issues exist to foster discussion, you are in no position to demand that a given discussion be closed and finished, no matter how right you feel your opinion to be.

[silkentrance]

@loganfsmyth it is not dismissive. I am trying to get things going again. I have reviewed every issue I commented and every issue I found so far is either outdated, or based on a personal learning experience or just invalid according to the spec.

[alexbyk]

@silkentrance You used decorator and decoratorFactory, that's why in you example it's clear when to use brackets and when not. If you change the name of your factory to something usefull, for example Injectable, 50% will use brackets, while 50% will not. So the half of the users will make a mistake. (it's very hard to trace such mistakes)

You can try to argue that it's possible to detect, when decorator are called as a factory, and when are called directrly by examining arguments. In that case pls read first messages above, I proved that it's not possible to make such detections and every "universal" decorator has a potential bug (a heisenbug)

I used an example from the angular2 docs (Injectable is a factory in your terms), but you can find real world usage in other popular frameworks: nobody uses "factory" suffix.

I'm repeating myself, but:

The simple thing can help us to avoid all of this: Consider @decorator not a function, but a syntax construction, and

@decorator Class {} is the same as @decorator() Class {}, and decorator part is always a higher order function (or a factory, as you named it)

[loganfsmyth]

@silkentrance Posting Please close as won't fix. in a discussion thread is dismissive. This isnt a big report, it is a discussion / feature request. I don't think this is something that should be included, and neither do you, I agree with you. By all means disagree. Discussion is important.

My complaint is that demanding that this be closed as wontfix achieves nothing but shut down discussion and makes it appear that your opinion should somehow be the final one.

No-one, you or me, is in a position to decide what does and does not get closed, beyond @wycats and TC39, and it is presumptuous of you to do so.

[silkentrance]

@loganfsmyth that is why i first answered to the issue in an rather elongated reply and then addressed the person to see whether it is fit to close this for being a won't fix issue.

It seems that the person is still rather pertinent in its assumption that 50% of the developers out there will get decorators and 50%, including that person, won't.

[alexbyk]

I've rewritten the initial message to make my proposal more formal and clear

[silkentrance]

@alexbyk

please consider the following

function decoratorImpl() {}

function decorator(foo, bar)
{
   return decoratorImpl();
}

Here, the decorator returns the actual implementation of the decorator based on the parameters passed to it

class Foo
{
    @decorator('1', '2')
    foo() {}
}

The runtime will then use the decoratorImpl() returned by decorator(...) to decorate the method foo().

What is so complicated about figuring out the two different use cases of having decorators that do not take any additional parameters and decorators that act like factories and will take additional parameters. in turn returning the actual decorator function?

And furthermore, according to the API documentation of these decorators, (not) using these with parentheses when decorating a class, method or property thereof.

[alexbyk]

@silkentrance You gave an example which my proposal doesn't affect at all. My proposal affects only this case:

class Foo {
    @decorator foo() {}
}

While your example above will work without changes

[alexbyk]

@silkentrance and answering your question "What's so comlicated...".

I'm using framework Angular2. Which has @Injectable. I don't remember how to use it: @Injectable() class MyClass {} or @Injectable class MyClass {}.

Right now I should open a documentation and spend a time, because it's not obvious.

With my proposal I don't need spend extra time, because it doesn't matter. So here is a reason: to make things simpler.

[silkentrance]

@alexbyk you can't be serious. is this a prank?

@loganfsmyth please see for yourself.

[alexbyk]

@silkentrance I don't understand why are you asking. I am serious. And I can assure you, simplifying things, trying to avoid future mistakes ans saving time is a serious stuff for every developer. And even saving 2 symbols in the syntax worth this discussion.

I gave a good reason in the comment above. My proposal will save 30 seconds for you if you ever try to write Angular 2 application using i'ts Injectable decorator. Don't you thing 30 seconds for every similar case is enough to not close this discussion?

[loganfsmyth]

@silkentrance

you can't be serious. is this a prank?

Do you expect comments like this to lead to well-reasoned discussion? Implying that the person you are arguing with is inept and joking does nothing but detract from the discussion.

Just say you disagree. Keep it civil. We as a community can do better.

[silkentrance]

@alexbyk there is no way for the transpiler or native compiler/interpreter to distinguish between a decorator function that takes additional parameters and will then return the actual decorator and a decorator that takes no parameters at all.

Please see for example the Python django framework and its suite of decorators or the language level decorators themselves.

Considering your proposal

@foobar()
class Foo {}

would require the decorator to be implemented as such

function foobar(target)
{
   if (!target)
   {
        return function foobar(target) {...};
   }
}

And you want to burden all developers of decorators with that extra amount of code where you as the user could simply use

@foobar
class Foo {}

?

[alexbyk]

@silkentrance could you please read my proposal one time more. Because I proposed to allow users to write

@foobar
class Foo {}

which is the same

@foobar()
class Foo {}

And looks like we're discussing different things because you didn't read it carefully.

About compolers/implementation part you are wrong too. Because right now they handle 2 cases, while I'm proposing to left only one (and make the work simpler, or, at least, not more complex)

[silkentrance]

@alexbyk have you ever implemented a decorator yourself?

[alexbyk]

@silkentrance of course i have. And I'm not going to ask "have you?" because spamming this discussion with worthless questions won't make difference.

[silkentrance]

@alexbyk I disagree :grinning: see for example https://github.com/jayphelps/core-decorators.js/pull/51 for a decorator that uses some kind of "meta programming" to get things working, limited as they may.

Now, how would you realize your proposal with that decorator? Well, it is rather easy, but still, it requires additional and mostly redundant effort on behalf of the developer of that decorator. The emphasis here lies on redundant effort.

[alexbyk]

@silkentrance I made a source code review of core-decorators repo and can say "it contains a lot of bugs". I also made a step-by-step guide which describes where to find those bugs here https://github.com/alexbyk/decorators-flaw . I assure you after reading this guide step-by-step you will understand the whole picture and will go to teach maintainers of core-decorators repo. So give it a try.

[chocolateboy]

@loganfsmyth not (necessarily) disagreeing with your other points. I've barely glanced at the thread, but:

Nowhere else in js does a property or identifier access magically become a call.

getters and setters?

[silkentrance]

Is this a duplicate of #23?

[silkentrance]

@alexbyk For what it is worth, you are definitely putting some effort into this. So, 50% of the developer community do not get how things work with decorators, which is you. And the other half of the developer community which is 99.9999%, so far has not even chimed in on this prank. And, seeing your example REPL, sorry REPO, I must say that you did a good job. But when it came to 2.b, you failed miserably, again, understanding how decorators works.

@loganfsmyth sorry, but this cannot go unpunished...

[loganfsmyth]

@silkentrance What punishment? All I see here is you calling this person an idiot instead of responding with a reasoned response. This entire discussion is useless, you've convinced yourself that they can't possibly be on your level and are being extremely disrespectful. You disagree, we get it.

[alexbyk]

@silkentrance I think you should relax. As far as I can see you still don't get the original problem. I just pointed you that the library you mentioned above contains bugs and is more complex that it should be (complex means "bad quality, a lot of messy code"), and can be use as a big +1 for my proposal. But looks like you don't see it.

Did you read my guide to the final part? Because it's not "How to write..", it's "Hot you shouldn't write..." guide. Anti-guide. And you are now in the 3) phase (because your were using core-decorators which is exactly the 3) phase). When you get to the Final) you will understand how wrong you were. So instead of blah-blah-blah try to work harder and become smarter. I'm sure ur a good guy but right now you have some problems that cloud your mind. And if you want to continue this conversation: "think twice, then write (order matters)"

[loganfsmyth]

@alexbyk I think my feeling on all of this is that we could argue about this all day. I agree, it's not necessarily obvious that you need parens on some decorator functions because some decorators are actual decorators and some are decorator factories. It seems like something people will just need to learn, or library developers should keep this kind of thing in mind. People will learn.

Referencing a value and calling a function are different things, that is how JS behaves, introducing a difference makes the language inconsistent and just introduces the opposite problem. If I write code like this that conforms to your proposal:

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

class Foo {
    @decorator
    method(){}
}

and I decide to refactor that code with a wrapper function

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

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

I suddenly need two pairs of parens on makeDecorator, to me that's even worse, because it's super unexpected and people would get just as confused.

[alexbyk]

@loganfsmyth

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

this variant is invalid according to my proposal.

only this one is valid:

function decorator(){
    return (target, prop, desc) => {
      console.log(target, prop, desc)
    };
}

And the usage of it doesnt change. So this code will work as expected

That's the point. If we have only one variant of writing decorators, authors will NEVER need refactor the code and never make errors

[loganfsmyth]

@alexbyk your valid example is the same as mine. I get your example, that is why I went with one more level of nesting. If you nest twice, you end up going from no parens to two sets of parens.

[alexbyk]

@silkentrance Do you mind to write some simple exercise? It will be simple, and contains 3 steps. I assure you will understand all by yourself after step 3.

Try to write a class decorator that adds "greet" method to the class (write "Hello" to the console) and can be used this way. And say when you're ready (show your code)

var foo = new Class();
foo.greet(); // Hello.

[alexbyk]

@loganfsmyth yes, my valid example is the same as yours. But I don't understand what do you mean about nesting twice? Because you won't need 2 parentheses:

A decorator as you wrote it is valid

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

And usage after my proposal: all these variants are valid:

class Foo {
    @makeDecorator() // emtry args
    method(){}
}

And this too

class Foo {
    @makeDecorator // no parens means  (), so this variant is equalt to the variant above
     method(){}
}

And when we want to pass some arguments, we don't need to refactor 'makeDecorator', it just works as expected

class Foo {
    @makeDecorator(1,2,3) // some arguments
    method(){}
}

So we would better use just decorator instead of makeDecorator.

P.S.: Right now you can archive the same result by examining the arguments. Like the most of libraries does. Stop. You can't. So the most of libs right now contains a bug

[alexbyk]

@loganfsmyth I made an example when you should use 2 pairs of parens, it works the same way with my proposal or without, and doesn't make sense for me . Also this function isn neither decorator, no decoratorFactory. Is that what you mean?

But my proposal doesn't affect (2)decoratorFactory, it only removes (1), so all decorators written the "right way"(2) will be working without changes. Decorators written like this:

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

works right now, but become invalid and should be rewritten according to my proposal

[loganfsmyth]

@alexbyk My argument is not that it doesn't work, I'm just saying that you are trading unexpected behavior at one side for unexpected behavior on the other side. Rather than having to think of "do I use parens on this decorator or not" in the current proposal, in my example you have to remember "do I use one set of parens or two sets".

[silkentrance]

@loganfsmyth I am not calling anyone stupid. But, see, your´s and possibly also my explanations are rather at the meta level which maybe these folks do not get their heads around very well. So perhaps, employing a Meinungsverstärker would do the trick? JUST JOKING.

[loganfsmyth]

You say

I am not calling anyone stupid.

but then

maybe these folks do not get their heads around very well

says exactly the opposite. Everything about your responses and your attitude indicates that you consider yourself to be way smarter than the OP.