-
Notifications
You must be signed in to change notification settings - Fork 127
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
Proposal: Allow client code to intercept the decoration process #65
Comments
@silkentrance @lukescott @isiahmeadows @alexbyk @jayphelps |
Will stew on it. One minor typo: deco.call(ctx, ...args) {
return deco().call(ctx, ...args);
}
// should be
deco.call = function (ctx, ...args) {
return deco().call(ctx, ...args);
}; |
@Download Maybe // Original
class Foo {
@decorator
bar() {}
}
// My proposed desugaring, where $variables are purely internal:
class Foo {
bar() {}
}
let $hasCall = Object.prototype.hasOwnProperty.call(decorator, Symbol.call);
let $desc = Object.getOwnPropertyDescriptor(Foo.prototype, "bar");
let $ref = $hasCall ?
decorator[Symbol.call](Foo.prototype, "bar", $desc) :
decorator(Foo.prototype, "bar", $desc);
Object.defineProperty(Foo.prototype, "bar", $ref !== undefined ? $ref : $desc); |
call is a built in function. Overriding it in this fashion is confusing. Symbol.call is for making a class callable without 'new'. At that point why not make it a class? As I've said before I believe backwards compatibility is an unreasonable constraint just because people jumped the gun on a stage 0/1 proposal. I would rather do this right then be limited by bad decisions. |
@lukescott I'm not that tied to |
@isiahmeadows Note that we can already override I'm trying to look for a scenario where just always invoking @lukescott The decorators proposal is defining a whole new symbol with Again, overriding @jayphelps |
@Download I think I see where you're going with this, but what's For others that overlook some of the more minute details of this idea, it's the difference between this ( // Original
class C {
@decorator
method() {}
}
// Transpiled now
class C {
method() {}
}
var %func = decorator
var %desc = Object.getOwnPropertyDescriptor(C.prototype, "method")
%desc = %func(C.prototype, "method", %desc) || %desc
Object.defineProperty(C.prototype, "method", %desc)
// Proposed here
class C {
method() {}
}
var %func = decorator
var %desc = Object.getOwnPropertyDescriptor(C.prototype, "method")
%desc = %func.call(C.prototype, "method", %desc) || %desc // Here's the difference
Object.defineProperty(C.prototype, "method", %desc) My main concern is with TypeScript, which might result in type conflicts once variadic generics are implemented (it's a beast). interface Function<R, T, ...A> {
call(thisArg: T, args: [...A]): R;
}
type MethodDecorator<T> = (target: Object, propertyKey: string | symbol, descriptor: TypedPropertyDescriptor<T>) => TypedPropertyDescriptor<T> | void
interface Decorator<T, ...A> extends (...args: [...A]) => MethodDecorator<T> {
// inherited with [...A] = [Object, string | symbol, TypedPropertyDescriptor<T>]:
// call(thisArg: any, arg0: Object, arg1: string | symbol, arg2: TypedPropertyDescriptor<T>): MethodDecorator<T>
call(thisArg: any, arg0: Object, arg1: string | symbol, arg2: TypedPropertyDescriptor<T>): TypedPropertyDescriptor<T> | void
} That return type will fail to check, because the types are wholly incompatible. That's my greater concern, since decorators are of equally large interest to the TypeScript community (they're looking into integrating them more deeply than even JS proper, if you look at their roadmap). To be honest, TypeScript compatibility is probably the only reason I can't support this idea. |
@isiahmeadows Yes the way your example transpiles is exactly what I had in mind. Would there be a way around the issue you are describing?
If you mean what |
Why I suggested a symbol. TypeScript understands those perfectly, and it's very easy to check for the existence of such a symbol ( This also is more behaviorally and logically consistent with the rest of ES2015+, where magic methods and values are all denoted by symbols (like What it would desugar to, then, is this: // Original
@Class
class C {
@Method
foo() {}
}
// New
function %decorateClass(target, decorator) {
if (Object.prototype.hasOwnProperty.call(decorator, Symbol.call)) {
decorator = decorator[Symbol.call]()
}
return decorator(target) || target
}
function %decorateProperty(target, property, decorator) {
if (Object.prototype.hasOwnProperty.call(decorator, Symbol.call)) {
decorator = decorator[Symbol.call]()
}
const desc = Object.getOwnPropertyDescriptor(target, property)
Object.defineProperty(target, property, decorator(target, property, desc) || desc)
}
class C {
method() {}
}
%decorateClass(C, Class)
%decorateProperty(C.prototype, "foo", Method)
This is pretty much what I expected. I was just double-checking. // I like the idea of this:
function universal(wrapper) {
wrapper[Symbol.call] = wrapper
return wrapper
}
const Injectable = universal((opts = {}) => (target, prop, desc) => {
// do things...
})
// Or if you *really* need a class...and no, inheritance doesn't work.
function decorator(D) {
function decorator(opts = {}) {
const d = new D(opts)
d[Symbol.call] = function () {
return (...args) => this.apply(...args)
}
return d
}
Object.setPrototypeOf(decorator, D)
decorator.prototype = D.prototype
decorator[Symbol.call] = () => new D()[Symbol.call]()
Object.defineProperty(decorator, "name",
Object.getOwnPropertyDescriptor(D, "name"))
return decorator
}
@decorator
class Injectable {
constructor(opts) {
this.opts = opts
}
apply(target, prop, desc) {
// do things...
}
} |
@isiahmeadows Those are some cool ideas. I have nothing against using a symbol. I just did not see any need for it. It seems like a relatively small change. As long as the end result is that we can intercept the decoration call I would be very happy with it. A bit off-topic, but why would you write it like this: Object.prototype.hasOwnProperty.call(decorator, Symbol.call) ...and not like this? decorator.hasOwnProperty(Symbol.call) EDIT: had some trouble finding relevant info on |
@Download This. 😄 // Totally valid JS:
decorator.hasOwnProperty = function (what, the, heck) {
return what + the + heck
}
decorator.hasOwnProperty("somethingThatDoesn'tExist") === "somethingThatDoesn'tExistundefinedundefined" Or in all seriousness, the object can define its own variant, and thus it no longer actually checks what you think it is. As for |
@isiahmeadows
That explains why I can't find it! :) I assumed it existed because of @lukescott 's comment that
Do you have a link where I can learn more about that maybe Luke? |
@lukescott If you're referring to the call constructor proposal, that's now using specific syntax instead. class Foo {
call constructor(whatever) {
// do stuff...
}
} |
@Download @isiahmeadows The problem I have with this approach is that the author of the decorator must define the decorator using two distinct steps. In addition, Isiah just proposed adding the well known symbol With #62, the definition of a decorator is far more simple and with the introduction of the DecorationDescriptor, even #68 can be fixed, always requiring the decorator to return an instance of a well known object, namely the DecorationDescriptor. This would also fit well into the TypeScript world... |
Edit: add some explanation @silkentrance 👎 for adding a whole new type to the language. Like we need more globals. I don't think we need an extra type to nominally check. My idea of a And for yours being conceptually simpler, I disagree. IMHO, 2 "steps" is simpler than 1 here. // Yours
function pdecorator(optionsOrDescriptor) {
let descriptor;
let options = optionsOrDescriptor;
if (optionsOrDescriptor instanceof DecorationDescriptor) {
descriptor = optionsOrDescriptor;
options = makeDefaultOptions({});
} else {
options = mergeUserOptions(options);
}
function decoratorImpl(descriptor) {
// make use of options here...
}
if (descriptor instanceof DecorationDescriptor) {
return decoratorImpl(descriptor);
}
return decoratorImpl;
}
// Mine
function pdescriptor(options = makeDefaultOptions({})) {
options = mergeUserOptions(options);
// decorator implementation
return (target, prop, descriptor) => {
// make use of options here...
};
}
pdescriptor[Symbol.call] = pdescriptor; For a proper comparison, here's what I see. Yours:
Mine:
|
And I also just realized an added benefit for this: you could actually properly use curried functions as decorators by using function curry(f, len = f.length) {
if (len === 0) return f
function result(...args) {
if (args.length === 0) return result
if (args.length >= len) return f(...args)
return curry((...rest) => f(...args, ...rest), len + args.length)
}
// Fully apply this if using it as a decorator
result[Symbol.call] = () => f
return result
}
const decorator = curry((name, opts, target, prop, desc) => {
if (prop == null) {
// handle class
} else {
// handle property
}
})
@decorator("Foo class")({some: "options"})
class Foo {} |
As far as overriding Using var decorator = {
decorate: function(target, prop, desc) {
// ...
}
};
function decoratorFactory(args) {
return {
decorate: function(target, prop, desc) {
// ...
}
};
} It doesn't have to be exactly like the above, but what I'm getting at is the decorator doesn't have to be a function. As far as "backwards compatibility" is concerned, you could easily do a typeof check and support the old style and display a warning to upgrade: // code used by compiler (babel,etc.) runtime which can be simplified at a later date
function decorateHelper(decorator, target, prop, desc) {
if (typeof decorator === "function") {
console.warn("old style decorator used. will be removed in future. please upgrade!");
return decorator(target, prop, desc);
}
return decorator.decorate(target, prop, desc);
} I love being able to use separate functions instead of ifs, as this issue suggests, so I would like to extend the above to something like: var decorator = {
decorateMethod: function(descriptor, prop, protoOrClass) {
// ...
return descriptor;
},
decorateClass: function(Class) {
// ...
return Class;
}
}; Doesn't have to be exactly like that, but the idea is that there is a different function depending on the context in which the decorator is used. |
@lukescott That's not much different than my/@Download's current proposal, if you're relying on methods (as this proposal does). The only difference is more verbosity for less duck typing. // Yours
function decorator(opts = {}) {
return {
decorateMethod: (desc, prop, proto) => {},
decorateClass: Class => {},
}
}
decorator.decorateMethod = (...args) => decorator().decorateMethod(...args)
decorator.decorateClass = (...args) => decorator().decorateClass(...args)
// Mine
function decorator(opts = {}) {
return (target, prop, desc) => {}
}
decorator[Symbol.call] = decorator |
@isiahmeadows You have won me over. I especially like the fact that Symbol.call does not yet exist. We could have a debate on what the best symbol name would be (e.g. we could opt for The currying example you showed is another cool idea! I think it shows that having a mechanism to allow authors to intercept the decoration mechanism opens up a whole range of useful options. |
@isiahmeadows The difference is I'm suggesting the decorator should be an object, not a function. And for "backwards compatibility" a runtime The decoratorMethod and decoratorClass thing was an extension. To allow a decorator to work in both contexts without an if statement. |
call
method
@isiahmeadows I updated this proposal to reflect your remarks on using a well-known symbol i.s.o. overriding the |
@lukescott That's a whole separate proposal, mostly unrelated to this (or even the original #23). It deals with the fundamental type of a decorator in general, where this deals with @Download Okay. 😄 |
@isiahmeadows the proposal in #62 was born from an idea by @jayphelps that one needs to duck type the existing arguments to find out whether one is decorating a class, or an instance property or method and so on. Therefore the extra built-in type. And as far as 'currying' goes, the proposal does not set any limits to it 😃 But we should stay on topic here and discuss this over at #62 if you will. Regardless of that, introducing a new Symbol.decoratorCall is not very different and requires both the engine and the transpilers to be aware of such a built-in Symbol.decoratorCall. Whereas in opposite of the DecorationDescriptor the new Symbol.decoratorCall would be less flexible as one is unable to determine the exact nature of the decoration, leaving us again with the duck typing process finding out what it is that we need to decorate. Which can actually be overcome by the proposal found in #62. And since this is an alternate proposal to that, this can also be considered taken care of, with less hassle on both the engine/transpiler part and the implementation of the decorator. |
Pretty much what I said about it not being fully relevant to this. As for the duck typing, I don't have as much of a problem with it. It's On Thu, Mar 31, 2016, 14:55 Carsten Klein [email protected] wrote:
|
@isiahmeadows now, at first this issue was about not being required to duck type, then, on my input this was made more universal and rightly so. And the original was exactly about people neither reading nor remembering API docs and being unsure when to apply @decorator([optionals]) and when to simply use @decorator and to leverage that situation by making both @decorator([optionals]) and @decorator the same. Hence this proposal and the proposal found in #62. In either case, the proposal found in #62 is more sound. And not just because I made it, but simply because it requires less effort by both the authors of decorators, regarding one having to additionally define decorator[Symbol.decoratorCall] with this proposal and the implementors of both engines and transpilers having to cope with that extra indirection. In addition, the proposal in #62 also will eliminate said duck typing. And not considering my introducing statement in there, that I am happy with the situation as it currently is. |
True. This is why I originally proposed just allowing client code to override
Again true. And again I'm with Isiah on this that it's just a much rarer use case and that it's not worth sacrificing the simple elegance of a simple function accepting plain arguments for it. That said, I don't think the two proposals are actually incompatible. Even if #62 were adopted, it would probably still be powerful to allow the decoration process itself to be trapped. The biggest difference for me is that this proposal is backwards compatible. Which means that adding it will not break any existing code. As I am seeing decorators used in lots of places already (and regularly find myself writing them) I think we need a really good reason to break back compat. We shouldn't just because we can (because this is stage 1). Instead we should only do it if we have a really important reason that cannot be avoided otherwise. That is my opinion. |
@Download breaking backwards compatibility with a stage 1 feature is a simple to do thing. just consider the transition from babel 5 to babel 6 and how many formerly working packages had to be adapted in order to make them work again. as such I consider this a non argument. Also considering https://github.com/jayphelps/core-decorators.js#future-compatibility and people are already expecting fundamental changes. |
But every time it happens people get hurt. Those updates cost time and money. It will make people more wary of adopting new features if it happens too much. Standards creation is a delicate process. Look what happened with HTML. It just got stuck for almost a decade. What we need is market pressure. There should be so much code out there using decorators that offering them natively will become a selling point. Then, when they become available natively on some platforms, more people start using them, increasing the pressure on other vendors to follow suit. But if there is hardly any code using it, vendors might hold off on implementing them natively, which in turn may cause devs to hold off on using them. There is a real chicken and egg risk. Just because some committee ratifies some standard does not mean it will actually be implemented and used. I'm just a huge fan of decorators and want them to thrive. I think that will happen faster if we avoid breaking code from early adopters. Nothing kills the joy of using some cutting-edge tech than having it fail on you frequently and being questioned (e.g. by management or even just peers) on why you are using experimental features. |
@Download sorry, but I was still working on the post, see #65 (comment) for an addition. |
I think we'll have to agree to disagree here :) |
@Download OFFTOPIC considering that HTML was stuck in that dilemma was just caused by market pressure. What we need is a future proof and extensible solution. One that even the peops over at core-decorators.js and similar such frameworks, and babel and TypeScript can live with. Not to mention engine developers over at Mozilla, Apple and Google or elsewhere. And while I still believe that the current proposal for decorators is a very sane and future proof approach, lest the expense of duck typing and reading API docs (see #23), I think that this approach is something that will cause people to refrain from implementing decorators. The same is true for #62, of course, as it adds additional complexity to a decorator implementation. And all of that extra complexity stemming from #23 and the "inability" of oneself to memorize API docs or failing to properly analyze/debug existing code. |
I would have to disagree about the added cognitive load in this vs your proposal. I've already addressed most of why I disagree yours is conceptually simpler, but here's a gist comparing the two approaches. It appears that in practice, there's not a ton of difference between the two. Yours actually surprisingly requires a little more duck typing in the decorator itself, from what I found. As for the current state of the decorator proposal, I wouldn't feel bad if nothing is done about it here. I can always send an almost-trivial PR Angular's and Aurelia's way if this/yours end up not panning out. (It's okay to TL;DR past this part. 😉) If anything, I did notice that in yours, there's a little bit of boilerplate for yours that basically boils down to something like this (what I used throughout that gist): export default function decorate(...args) {
if (args[0] instanceof DecorationDescriptor) {
return reallyDecorate(args[0])
} else {
return dd => reallyDecorate(dd, ...args)
}
}
function reallyDecorate(dd, ...args) {
// do things
} Alternately, you could do something like this: export default function decorate(dd, ...args) {
if (!(dd instanceof DecorationDescriptor)) {
return dd1 => decorate(dd1, dd, ...args)
}
// do things
} Mine effectively ends up something like this: decorate[Symbol.decoratorCall] = decorate
export default function decorate(...args) {
return (target, prop, descriptor) => {
// do things
}
} |
@Download see https://github.com/tc39/proposal-decorators for the new spec proposal. |
@silkentrance Thanks for the heads-up! |
TL;DR;
Allow client code to intercept the decoration process by defining a well-known
Symbol.decoratorCall
. When the runtime detects a property on a decorator with this symbol as it's name and a function as it's value, the runtime will invoke that function i.s.o. the decorator function itself to perform the decoration.Background
There has been a lot of discussion around the way decorators are used and how the spec allows us to write code for these scenarios. Mostly, there are two ways to use decorators that are used a lot:
So far so good, but what happens if the decorator accepts an optional argument? We find that whether we supply parentheses or not makes a difference. In other words, these two uses of decorators are not equivalent:
The big difference is that in the first scenario
deco
is invoked as a decorator and is expected to return the decoration target (e.g. the class in this example), whereas in the second scenario,deco
will first be called without any arguments and is expected to return a function, which will then be invoked as the decorator. We will call this a decorator factory.Many authors have expressed the desire to have
@deco
and@deco()
have the same outcome: The target is decorated withdeco
without any arguments. This can be achieved with anif
inside the decorator that uses duck typing to determine whether the function is invoked as a decorator or as a decorator factory:This function will handle all scenarios described above. I will refer to functions that work this way as 'universal decorators'. They are convenient. But this code contains some problems:
if
is brittle. What if we want to write a class decorator that optionally accepts a function as an argument? In some cases, it may be impossible to distinguish between decorator and decorator factory invocations.The discussion around this topic has spawned some interesting ideas for solutions, the most extreme of which would break all existing decorators. This proposal attempts to formulate a solution to these problems that is fully backwards compatible.
Hooking into the decoration process
The
if
in our universal decorator example above is really what is causing us all our grief. It splits our code in two and worse, the condition it is checking is brittle and will often have to change depending on what arguments our function accepts. What if we could get rid of thisif
altogether?Turns out we could. And best of all, we could do it without breaking backward compatibility. As a bonus we would be creating a generic mechanism that could support other scenarios besides writing universal decorators. We can allow authors to hook into the decoration process itself.
Symbol.decoratorCall
In Javascript, functions are objects. Which means they can have properties. We can use this to allow authors to set a property on a decorator function in such a way that they can trap the decoration process. Given a well-known symbol
Symbol.decoratorCall
, authors could set a property on their decorator using this symbol to set a function to be called by the runtime instead of the decorator function itself. As authors, we now have an extra tool that can help us rewrite our universal decorator to something much simpler:Notice how the function assigned to the
Symbol.decoratorCall
property is completely generic. We as authors can take advantage of this fact to implement our own tooling for universal decorators:Then, we could write only the decorator factory and let the tooling turn it into a universal decorator for us:
If we ever get decorators on functions, we can make it even nicer :)
Conclusion
Although this proposal was born out of the desire to be able to write universal decorators more easily and to avoid duck typing, I think that being able to intercept the decoration process will actually open up options to deal with other issues as well.
Advantages:
Symbol
. E.g.Symbol.iterator
which can make our object work with thefor ... of
loop.Disadvantages:
The text was updated successfully, but these errors were encountered: