wycats / javascript-decorators

2.4k stars 127 forks source link

Declaring a decorator so it is context aware #40

Open lukescott opened 9 years ago

lukescott commented 9 years ago

I have a case where I want to use the same decorator, but do something slightly different depending on the context. For example:

@attribute("value", {"default": "foo"}) // Adds a default get/set for value and registers it as an attribute
class Foo {
  @attribute({"default": 0}) // registers size as an attribute and adds a default value
  get size() {
    return this._size
  }

  set size(size) {
    this._size = size;
  }

  get name() {
      return this._name;
  }

  @attribute // registers name as an attribute
  set name(name) {
    this._name = name;
  }
}

The problem is in order to make all three work I need to use 3 different decorator syntaxes:

// class Foo case
function attribute(name, options = {}) {
  return function(constructor) {
    ///...
  }
}
// get size() case
function attribute(name, options = {}) {
  return function(prototype, name, descriptor) {
    ///...
  }
}
// get name() case
function attribute(prototype, name, descriptor) {
  // ...
}

Detecting the context and using the proper declaration is a bit difficult and cumbersome. There are two issues:

1 - @attribute and @attribute() have two different decorator signatures. First is the decorator the second returns a decorator. They both should be the same. And @attribute(name, options) should also work with the same definition. 2 - There is no indication of context, unless you check the argument types, which can be messy considering that the prototype is an Object. If you wanted the first argument to be an Object how do you know it is a prototype or an argument?

A alternate for creating a decorator like this could work:

function attribute(...args) {
  return {
    class(constructor) {
      let [name, options] = args;
      // ...
    },
    get(prototype, name, descriptor) {
      let options = args[0];
      // ...
    }
  }
}

The decorator method would return an object containing functions for each context. Keys of the object could be "class", "get", "set", "method", "function", "var".

The reserved words actually work in babel when used in that way. If that doesn't work you could do something like "class": function(constructor){...} instead.

chocolateboy commented 8 years ago

This appears to be addressed by the latest draft of the spec.

loganfsmyth commented 8 years ago

This seems like it would introduce an ambiguity that exists nowhere else in the language. At least with the current proposal

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

and

let dec = decorator();
class Example {
    @dec
    method(){}
}

would be interchangable, which makes sense because

console.log(decorator());

is identical to

let dec = decorator();
console.log(dec);

It seems like you're just trying to overload attribute too much. If something has drastically different behavior, why not have multiple functions?

lukescott commented 8 years ago

@loganfsmyth As far as my use-case is concerned, I agree that I could take a different approach. What I was trying to highlight was how fragile the definition of decorators are. After some more thought, I think that decorators shouldn't be raw functions. At least as long as you can do @decorator vs @decorator().

What do you think about something like this?

// or new Decorator({...}) ?
var decorator = Object.createDecorator({
  // regardless of how decorator is used, @decorator or @decorator(), use the method
  // below and pass args when @decorator(...) is used
  classMethod: (target, name, descriptor, args = []) => {
    // do decorator stuff
    return descriptor; // should always return a descriptor
  }
});

The idea I'm trying to bring across is that how the decorator is used is important, and should be a different function. You shouldn't have to do argument length or type checks to figure out what the decorator should do. The parser already knows how the decorator is being used and should invoke the right code path for you.

silkentrance commented 8 years ago

@lukescott I guess that you have worked with Python and Python decorators in the past? Or even Java and Java annotations?

Except for Java, decorators/annotations are fragile and must be used in the context they have been designed for. It is up to the designer of that decorator to make sure that it gets applied correctly. And if the decorator fails to assert its parameters to prevent misuse, well, then it is up to the user to either report such errors to the original developer or adjust his own behavior and use the decorator as it is intended to be used.

While one could add a new language feature, say

@decorator.class
@decorator.method
@decorator.property
@decorator[.any]

and annotate a given decorator function with that, the compiler could make sure that a given decorator is applied to only the contexts it was defined for (1).

I'd say that this would be a nice extension to the language and would make the use of decorators more safe for the user.

Yet, during compile time this may be checked for the same code basis only. Once you require a module from a third party package, everything must be checked during runtime again.

As such, the guards implemented by above meta decorator mechanism and the guards that should be implemented by the developer of the decorator are actually the same.

And what about parameterized decorator (factories)?

silkentrance commented 8 years ago

Close, impossible to do?

lukescott commented 8 years ago

Impossible? No. The parser knows what context the decorator is being used. It can easily call a different method depending on the context.

silkentrance commented 8 years ago

@lukescott yes, sure, but this would require the transpiler to know forefront which types of functions/methods are available. And considering one using decorators from an external library, this seems rather far fetched.

Unlike Java or Python, the babel transpiler does not load nor otherwise investigate the external imports and thus will never know what is implemented in there. As such, this proposal requires some fundamental changes to how the babel transpiler works, e.g. loading external sources and trying to parse them although they are not valid es6 code etc.

silkentrance commented 8 years ago

@lukescott since overloading a simple function seems to be a stresser here, see for example https://github.com/jayphelps/core-decorators.js/pull/51. And while that PR might never be integrated, it should show you a path that you can use for implementing decorators that will serve multiple use cases.

And in your case, what you actually need is a decorator factory, one that will return a decorator function. So in your last example you would have to use @attribute() set name() instead.

As @loganfsmyth put it, you best bet would be to use different decorators or implement your decorator factory so that it is able to adopt to the different use cases.

lukescott commented 8 years ago

The original use-case of using the same decorator in both class and method context may not be the best approach. However, the current implementing does not stop someone from trying to use a class method decorator class or vise versa. The decorator has to somehow check for that.

If a decorator was defined via an Object.createDecorator({...}) or new Decorator({...}) you could define handlers for the different cases. And if you don't specify a case for the decorator the engine can throw an error. You don't have to pollute your decorator handler at that point. And on top of that you can distinguish a decorator from a regular function, which at the moment you can't.

silkentrance commented 8 years ago

see #62 and possibly also #65