Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More fine-grained builtin decorators #298

Closed
bergus opened this issue Nov 16, 2019 · 1 comment
Closed

More fine-grained builtin decorators #298

bergus opened this issue Nov 16, 2019 · 1 comment

Comments

@bergus
Copy link

bergus commented Nov 16, 2019

Something that is not explained in the readme and for which I am currently lacking a mental model is how a decorator can distinguish what kind of element it is applied on and decide what/whether to do with it. It seems that @wrap, @initialise and @register can be put on fields, methods, or the whole class, and the callback would then be called with different arguments.

I don't think that's a good idea. If I write a custom decorator, I would need to overload my callback to deal with all those cases. For example, lets say I write a simple method logging decorator that @wraps methods and logs their name along with their arguments. But this decorator makes no sense on a class (that doesn't even have a method name), so I would need guard my code against this usage. Similarly, to @register a static method somewhere, I would have to check that the target is a class, not a prototype object, because I would never want my registration to invoke a method on the prototype.

This is related to #275 - what happens when I try to @wrap a field, which is documented to be impossible (for now)? Does the decorator simply get ignored? Does the usage produce an error? I would hope the code fails (to compile/to run) with a warning for the author, just like when my method-logging decorator is used in the wrong place.

What I would suggest is to make the builtin decorators much more fine-grained. It seems the issue was already noticed: "@expose is separated from @register to reduce the risk […, although] technically speaking, these extra arguments could be passed into @register as additional arguments instead". I would wish for more separation here, at least by kind and placement.
We still could have combinations of them, like

decorator @wrapMethod(w) { @wrapPrototypeMethod(w) @wrapStaticMethod(w) }
decorator @wrap(w) { @wrapMethod(w) @wrapClass(w) }

For example, an autobind decorator would become much more declarative by this:

decorator @autobind {
  @initializePrototypeMethod((instance, name) => {
    instance[name] = instance[name].bind(instance);
  })
  @wrapClass((cls) => {
    for (const m of Object.getOwnPropertyNames(cls.prototype))
      Object.defineProperty(…) // make a lazy getter
  })
}

If used on a method, the initialize would happen and the wrap would be ignored, if used on a class, the wrap would happen and the initialise would be ignored, if used on a field or static method, none of the decorators would apply and it should cause a compile error (we don't want it to be silently ignored!). All without the callbacks having to figure any of this out themselves.

I believe this also improves future extensibility while keeping compatibility. We would not change the builtin decorators to apply at more locations than before, which could cause existing usage to break, but instead we would add more of the atomic builtin decorators.

@littledan
Copy link
Member

The current proposal does not use built-in decorators anymore; see README.md and #310.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants