-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[FEAT] Implements Decorators #17548
[FEAT] Implements Decorators #17548
Conversation
a49dbb5
to
443e536
Compare
Thanks @richard-viney! |
0cebdcb
to
4359400
Compare
Benchmarked, no statistically significant change in performance when used in classic class syntax! I think this is ready for review 🎉 |
99d5213
to
df39f05
Compare
@type DOMElement | ||
@public | ||
*/ | ||
element: descriptor({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Followup: We should flatten this class into Component
so we don't need to use this decorator
6f95d66
to
3ecf45b
Compare
93d42b1
to
d3a6a24
Compare
This PR refactors the Descriptor system in Ember metal, mostly replacing it with a Decorator system instead. `defineProperty` has been updated to apply decorators following the currently implemented spec. This means that the value returned by `computed` goes through the exact same flow when applied as a decorator using decorator syntax, and applied using classic syntax.
d3a6a24
to
53b84e2
Compare
|
||
constructor(config: ComputedPropertyConfig, opts?: ComputedPropertyOptions) { | ||
constructor(args: Array<string | ComputedPropertyConfig>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this change intentional? Shouldn't it be ...args
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed on Discord that it is intentional.
This is a continuation of #17226, thanks a ton to @NullVoxPopuli for getting this started!
This PR refactors the Descriptor system in Ember metal, mostly replacing
it with a Decorator system instead.
defineProperty
has been updated toapply decorators following the currently implemented spec. This means
that the value returned by
computed
goes through the exact same flowwhen applied as a decorator using decorator syntax, and applied using
classic syntax.
I was considering pulling out the ComputedDecorator stuff into a separate
package, it's relatively isolated from the rest of
metal
at this pointso it would make some sense. Thoughts?
TODO: