Decorator call signature ambiguity

[rbuckton]

This was previously discussed in the old repository, and was brought up during the last TC39 meeting.

If we are considering auto-inserting parentheses, we can generally modify the algorithm for DecoratorEvaluation to be:

Decorator : `@` DecoratorMemberExpression[?Yield]

1. Let expr be the result of reparsing DecoratorMemberExpression as a MemberExpression.
2. Let ref be the result of evaluating expr.
3. Let func be ? GetValue(ref).
4. Let value be ? EvaluateCall(func, ref, « », false).
5. Return value.

This would preserve the correct this, but we would need to articulate the difference between this:

@decorator   // evaluated as: decorator()(descriptor)
@decorator() // evaluated as: decorator()(descriptor)

And this:

@(decorator)   // evaluated as: (decorator)()(descriptor)
@(decorator()) // evaluated as: (decorator())()(descriptor)

As it aligns with the behavior of new:

function Foo() {}
new Foo();   // Foo {}
new Foo;     // Foo {}
new (Foo);   // Foo {}
new (Foo()); // TypeError: Foo(...) is not a constructor

[littledan]

I'm really confused by this issue. I don't know of any cases where just adding or removing parentheses around an expression changes the receiver like that. The cases I know of are more like this:

> ({x() { return this.y }, y: 1}).x()
< 1
> (0, ({x() { return this.y }, y: 1}).x)()
< undefined

The receiver semantics I'd expect for the paren auto-insertion case would be more like, when calling the decorator the first time, use the receiver that's textually there (if any), and for the second call, use undefined as the receiver. The first call can .bind() to provide a receiver for the second call, if this is important.

[rbuckton]

The only place this happens in ES is with new, as in my example above. I can look at changing the behavior to align more closely with call, but it's a larger spec change.

[littledan]

@rbuckton I'm not sure what you mean by a larger change. I like your semantics in #82. What else would the semantics be?

[k1r0s]

Where is the ambiguity?

Decorators are functions that needs to be declared within the evaluator expresión @ that will execute that expression and expects a function to be returned. So having parentheses or not depends on what kind of expression you need to build. Maybe you need parameters.

There is no ambiguity whatsoever

[k1r0s]

We should not encourage developers to use this within decorator scope because there should be no context. IMO this should be null or global obj

[littledan]

Since the meeting, a number of people including @jridgewell, @ljharb and @gsathya have expressed some reservations about the decision that weren't expressed in the meeting, among other people. I'd like to spend some more time thinking about all of this before we fully settle on the semantics we discussed in the meeting.

@k1r0s When we spoke about this issue, you had some interesting use cases where the parens issue comes up--it sounded like the new auto-insertion semantics made those cases less ergonomic. Could you explain more?

[jridgewell]

After researching it more, I'm meh with it. We just need to communicate clearly (many many many times) that decorators are two-part functions, a factory and an implementation:

function DecFactory() {
  return DecImpl;
}
function DecImpl(descriptor) {
}

@DecFactory;       // evaluates to DecImpl(descriptor)
@DecFactory();     // evaluates to DecImpl(descriptor)
@(DecFactory);     // evaluates to DecImpl(descriptor)
@(DecFactory());   // evaluates to DecImpl()(descriptor)
@(DecFactory())(); // evaluates to DecImpl()(descriptor)

Given that, the current spec is exactly like new's optional parenthesis (if we made new require a two-part function):

function FooFactory() {
  return FooImpl;
}
function FooImpl() {
}

new FooFactory;       // evaluates to FooImpl
new FooFactory();     // evaluates to FooImpl
new (FooFactory);     // evaluates to FooImpl
new (FooFactory());   // evaluates to FooImpl {}
new (FooFactory())(); // evaluates to FooImpl {}

[k1r0s]

Yes, so having for example this like:

function beforeInstance (...advices) {
  return function(t, k, descriptor) {
    ...
  }
} 

@beforeInstance(inject.args(AxiosProvider, StorageProvider))
class WhatEver {}

the only requirement is that expression to be evaluated as a function.

then we can asign this thing into an alias if we don't need to tweak it for other entities:

const InjectParams = (...args) => beforeInstance(inject.args(...args))
const Inject = beforeInstance(inject.args(AxiosProvider, StorageProvider))

@InjectProviders(AxiosProvider, StorageProvider)
class WhatEver1 {}

@Inject
class WhatEver2 {}

You can simplify expressions and its very handy.

Again, I don't see any ambiguity on current api. What I do see is developers having to write two functions when they don't need to.

[Jessidhia]

The aliasing use case is one that should be provided by the expression case (@(Inject)) instead of the identifier case.

Making the identifier case hard to misuse and not require defensive programming on every single decorator factory is the better approach here.

---

EDIT: It seems that I misunderstood one detail, in that @(foo) seems to be treated the same as @foo. I think that the Expression syntax would be more useful if it were to bypass the implicit parens.

// proposal
@foo      // call foo as a factory; same as @foo() and @(foo())
@foo()    // call foo as a factory; same as @(foo())
@foo(bar) // call foo as a factory with an argument; same as @(foo(bar))
@(expr)   // will call expr result directly with the descriptor instead of as a factory

If that's not feasible, then making it behave exactly like new is not the worst thing; I'd still prefer it over making @foo and @foo() behave differently.

[k1r0s]

I'd still prefer it over making @foo and @foo() behave differently.

For me, this is the same case as doing this:

function bar(arg1) {
  console.log(arg1);
}

function foo() {
  return "foo"
}

bar(foo);     // logs [Function foo]
bar(foo());   // logs "foo"

JavaScript developers understand this perfectly.

new keyword is a special case, isn't the same!

[Jessidhia]

It looks easy in tiny examples. Making @foo and @foo() behave differently will force every single factory to do defensive programming to ensure they are being used correctly, and will make type declarations more difficult if your factory has optional parameters.

[k1r0s]

For me, this is not enough argument in order to do API changes ..

Decorator expressions are outside of program logic, they are pretty simple to remember. I don't think people will be forced to defensive programming if they don't need to. And that's not even driven by decorator syntax.

And, in the other hand, decorators are here to avoid code duplication and encourage encapsulation so, afterall it makes sense to make some validations if you're gonna put that pattern in a single place to be used everywhere.

[k1r0s]

The best of decorators is that you have a lot of freedom as long as you provide a valid expression with the current working typescript version: k1r0s/kaop-ts#89 (comment)

I can provide a lot of examples.

I hope you're familiar working with decorators in production environments

[littledan]

@nicolo-ribaudo , @rbuckton and I discussed this issue. All of us would be OK with doing what the decorator champion group previously proposed of not inserting the parentheses. We'll have to discuss this again in a larger group which includes people who argue in favor of the other path.

[cztomsik]

So those parens are just because of being able to decorate with dynamic decorator? In that case I think explanatory variable is far better (more readable) choice:

const foo = somehowGenerateDecorator()

@foo(args)

vs.

@(somehowGenerateDecorator())(args)

Edit: apparently this is offtopic, sorry :-)

[nicolo-ribaudo]

Actually this issue is about the parentheses which contains the parameters ((args) in your example). The question is: should it be possible to omit them of they contain no parameters, so that @foo and @foo() do the same thing?

[k1r0s]

with the current spec (which im using):

Its posible as long you force users to keep a factory function that returns the decorator function even if they don't use parameters. And I'm not agree

[littledan]

@wycats @bterlson @rbuckton and I discussed this issue again. The consensus among that group has been to push back against inserting the parentheses, but to make a tweak to the previous branding strategy: Instead of multiple @@toStringTag values, there's just one that's shared among element/class descriptors. This is intended to make the brand check simpler.

Does anyone want to write the patches and docs updates for this change?

[nicolo-ribaudo]

Hi, do you think that babel/babel#7773 is still needed?

[littledan]

Many people have pointed out, if we think about static decorators, we may want to reopen this issue. Right now, I'm leading to auto-parens if we go with this alternative.

[littledan]

OK, now that we're back to function-based decorators given #310, re-closing this issue, to leave it with the resolution described in #81 (comment)