Class and element decorators have inconsistent interface

[jkrems]

While element decorators get passed a descriptor record ("named arguments") in DecorateElement:

Let result be ? Call(decorator, undefined, « previousDescriptor »).

In DecorateClass the proposal uses positional arguments:

Let result be ? Call(decorator, undefined, « previousConstructor, heritage, previousDescriptors »).

I really like the symmetry in element decorators (the return type and the argument type map to each other quite nicely). Would it be acceptable to change the signature of class decorators to be:

function myClassDecorator({ constructor, parent, elements }) {
  return { constructor, finisher, elements };
}

[silkentrance]

What is finisher, I cannot find it in the proposal. The current proposal expects the class decorator to return an object {constructor, elements}, so there is actually no symmetry here.

[jkrems]

Not sure what you mean, the proposal's DecorateClass returns finishers and allows class decorators to return one: https://github.com/tc39/proposal-decorators/blob/d74d256856b06c71189fcdbf4b826f99d1e6f10a/decorators.html#L631-L652

[silkentrance]

@jkrems just looked into the compiled spec document and into the source and both do not match... all references to finishers have been removed from the compiled spec... weird.

[jkrems]

@silkentrance Where are you looking? The published spec and the master branch agree and have finishers as part of DecorateClass. Maybe you have an outdated checkout for some reason..?

[silkentrance]

@jkrems I just checked out the repo and did an npm run build on it...

[jkrems]

@silkentrance Maybe this comment can help you: https://github.com/tc39/proposal-decorators/pull/17#issuecomment-236686574. It contains a nicer build command to generate the proposal html.

[jkrems]

After running the new build command (or the watch command), you can open decorators/index.html.

[jkrems]

Another motivating example - @Reflect.metadata:

// Replacement for the decorator that ships with current reflect-metadata
function metadata(key, value) {
  return function decorate(element) {
    if (element.kind === 'property') {
      element.finisher = function addMetaData(klass) {
        Reflect.defineMetadata(key, value, klass.prototype, element.key);
      };
    } else if (element.kind === 'class') {
      element.finisher = function addMetaData(klass) {
        Reflect.defineMetadata(key, value, klass, undefined);
      };
    }
    return element;
  };
}

[jkrems]

cc @rbuckton - is this realistically how Reflect.metadata would work in a stage-2-decorators world?

[HyeonuPark]

I also love the approach that member decorator has MemberDescriptor => MemberDescriptor form. So it should be nice if class decorator also has ClassDescriptor => ClassDescriptor form. And of course, it must be far much nicer if all variations of decorators like function expression/object literal decorator also follows the same rule.

This is my suggestion for such a future-proof approach

interface ClassMemberDescriptor {
  kind: "ClassProperty",
  // same as current interface
}

interface ClassDescriptor {
  kind: "Class",
  name?: string, // class name if specified
  constructor: function,
  parent?: class,
  members: ClassPropertyDescriptor[],
}

Anyway, what's the extras property in PropertyDescriptor for? I cannot found document for it.

[jkrems]

@HyeonuPark extras is used to allow property descriptors to generate more than one property (see: ClassDefinitionEvaluation). The typical use case would be "generate getter/setter and private property".

[jkrems]

P.S.: "ClassProperty" wouldn't be quite correct because property decorators are also allowed on object literals.

[HyeonuPark]

I think in that case the key should be "ObjectProperty", As behaviors of class property/class method/object property are not same.

[jkrems]

@HyeonuPark Do you have an example where it becomes a problem? Otherwise it sounds like an annoyance in the making ("Ugh, the author forgot to add || kind === 'ObjectProperty', so now I can't use this deprecate decorator...").

[HyeonuPark]

@jkrems Sorry for late response '~'

Separated ClassProperty related issue to #21 . Let's assume it's just Property for here.

How about the ClassDescriptor interface?

[jkrems]

The ClassDescriptor interface makes sense to me and is very close to how I implemented it in my babel transform - if you ignore the casing of kind (my mistake but I'm too lazy to fix it right now ;)).

Waiting on feedback by @wycats. Maybe there's a deeper reason for the current interface.

[jkrems]

@wycats Any thoughts on unifying the way class- and member decorators are working (e.g. making both take & return a descriptor)?

[kt3k]

Doesn't this commit take this issue's suggestion?

cc @bterlson

[jkrems]

Interesting, didn't see that commit. But it doesn't look like it's updating the decorator function algorithm to accept class descriptors, just moves stuff around. The algorithm still accepts the same "random" positional arguments and returns a descriptor:

        1. For each _decorator_ in _decorators_, in reverse list order do
          1. Let _result_ be ? Call(_decorator_, *undefined*, « _previousConstructor_, _heritage_, _previousDescriptors_ »).
          1. Let _previousConstructor_ be ? Get(_result_, `"constructor"`).
          1. Let _finisher_ be ? Get(_result_, *"finisher"*).

[Jessidhia]

The way the class decorators are specified also makes the return value kind of a "magic value". You have to remember to return a value with the constructor and elements key. The way the merging currently works would allow you to omit constructor, but omitting elements will result in a class with no members.

I also wonder about the purpose of the heritage argument -- you can receive it, but you can't modify or do anything useful with it.

[littledan]

Now, in DecorateConstructor, The heritage is gone and a descriptor is sent instead of positional arguments. Does this address the issue?

[littledan]

I believe the current interface is more similar between class and element decorators, as was requested here. Please reopen the issue if you have any concerns.