extendDescriptor must recursively check super classes/constructors for the first occurrence of 'key'
silkentrance opened this issue · 26 comments
https://github.com/jayphelps/core-decorators.js/blob/master/src/extendDescriptor.js#L5
will result in error when deriving from a hierarchy of super classes where the most recently derived from super class does not have the specified own property.
E.g.
class Base
{
get foo() {return this._foo;}
}
class Foo extends Base
{}
class Bar extends Foo
{
@extendDescriptor
set foo(value) {this._foo = 1;}
}
So, rather than just looking for super to have the specified property, it should recursively traverse the inheritance chain instead, returning the first matching own descriptor found.
See https://gist.github.com/silkentrance/21a7016933b0e7d7338f969ba928a457 for a first stab at this problem.
@jayphelps perhaps we should input this to the ongoing discussion about the "final" decorator specification over at @wycats. It seems that one also needs to have access to the constructor function instead of just the prototype, e.g.
decorator({constructor, prototype, attr, descriptor});
where prototype may be undefined in which case the descriptor and attr refer to the constructor instead, aka static properties/methods.
what do you think?
Assuming you have a stereotypically formed class prototype, the constructor is available in decorators still at prototype.constructor. Some of the other decorators use this. Thoughts?
@jayphelps thanks for the clarification, I didn't know that prototype.constructor was available at that time.
As for the implementation, I think that a similar approach to @abstract checking recursively whether it is still abstract should do the trick. See 162bea8#diff-c087668e328e3128ef729aa631ffbd67R79
@jayphelps beginning work on a PR for this now...
@jayphelps uh, oh, it seems that the @extendDescriptor are never run... tests are named extendDescriptor.js instead of extendDescriptor.spec.js
uh, oh, it seems that the @extendDescriptor are never run...
@silkentrance what do you mean?
@jayphelps see above... just updated the comment.
whoops!
@jayphelps yikes, do you want to fix the failing tests or should I include that with the PR?
@silkentrance your call. If you don't want to do it I definitely will, just can't right this second
@jayphelps well it is actually just two extraneous ; 😄
I will implement the required missing tests as well.
@jayphelps initialized properties are difficult in that they will be realized / defined only when the class gets instantiated and there is no superDesc whatsoever unless the instance was created.
Unless the decorator should also work on the instance as well... or, considering static initialized properties, on the class/constructor.
I am dropping support for (static) initialized properties ATM as I cannot find a viable solution for them right now.
And it becomes even more problematic with non configurable initialized properties.
@jayphelps regarding initialized properties: initialized properties will never be defined using Object.defineProperty().
So, this is actually impossible to do, for both initialized static and instance properties.
And the issue with the initialized properties not being defined using Object.defineProperty() might affect other decorators as well... Perhaps, and in that context, decorators are expected to decorate the initializer function only?
It seems that babel5 does not implement initialized properties according to the spec. In the spec it reads that the initialized property should be defined using Object.defineProperty(). Babel5, however, will stop short and just skip the Object.defineProperty() step.
@silkentrance which version of the spec? babel v5 implemented stage-0 spec AFAIK and it has seemed to be correct in regards to initializers from what I can tell, can you clarify? (if you are looking for input, if just documenting your progress feel free to ignore)
@jayphelps Oh, you are right. I did not see that _defineDecoratedPropertyDescriptor() actually calls upon Object.defineProperty(). Thanks for clearing this up.
So we actually do have a special case here. Initialized instance properties will be decorated during design time whereas the property will be defined in the constructor, via _defineDecoratedPropertyDescriptor(), every time that a new instance of the object is created.
So at the time that the decorator is run, it cannot have knowledge of the not yet defined property and it is thus not able to extend the super descriptor.
As for statically defined, initialized properties, they will be defined during construction of the class and there will be a super desc that can be used for extension.
So, what about the single special case? Simply drop the support for it and bail out or silently ignore the fact that one cannot extend the super descriptor that way?
@jayphelps see also the latest version of the PR which will silently ignore the error
However, I still believe this to be a flaw in the spec and the implementation thereof. The properties should be defined on the prototype at design time and not when the instance is created.
@jayphelps see also tc39/proposal-class-public-fields#38 (comment) where I propose a different approach for declaring initialized properties and which might solve existing problems with decorating initialized instance properties.
@silkentrance I'm freeing up some time to wrap my head around all these issues here and wycats/javascript-decorators and gonna discuss them with wycats. Will report back.
@jayphelps please invite me to that discussion with wycats when online, I'd like to know and talk about decorators in general, especially about my proposals which might be sound and fair.
@silkentrance what's your email?
@jayphelps I've set it to public now.
@silkentrance hmm not showing up for me. Can you email me at mine?
@jayphelps send you the address. and it should now show up on the profile, too.
Is this still an issue, if not, please close.