-
Notifications
You must be signed in to change notification settings - Fork 38
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
Support for multiple decorators #10
Conversation
01a6358
to
bad52ea
Compare
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.
Looking good, I like the direction of abstracting the EOProp
class, thinking maybe we can start adding some more classes to normalize the rest of the abstractions (in future PRs though).
Couple minor comments, once those are addressed we should be good to go
@className("enabled", "disabled") | ||
isEnabled = false; | ||
isEnabled() { |
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.
This will need to be made a native getter get isEnabled()
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.
Yeah, looks little complicated to make a decision whether to make native getter. Should it be just if the property has multiple decorators the value should be either simple value
or native getter
? 🤔
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.
If it is marked by @computed
, it will always need to be a native getter. @className
can also only ever be applied to a native getter, or a class field, neither works with methods.
If it is an @action
it should be a method. I don't believe there are any other method decorators.
In general, method decorators and accessor/field decorators are pretty mutually exclusive at this point. You don't have decorators that apply to both types, just one or the other.
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.
Hmm, that means the this transform is incorrect in the decorators.output.js
/**
Computed description
*/
@computed("fullName", "age", "country")
description() {
return `${this.get(
"fullName"
)}; Age: ${this.get("age")}; Country: ${this.get("country")}`;
}
It should be
/**
Computed description
*/
@computed("fullName", "age", "country")
get description() {
return `${this.get(
"fullName"
)}; Age: ${this.get("age")}; Country: ${this.get("country")}`;
}
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.
I think only action
and on
are method decorators
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.
ah, and @observes
, thanks for reminding me about those
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.
👍
return createBindingDecorators(j, decoratorName, instanceProp); | ||
} | ||
return createCallExpressionDecorators(j, decoratorName, instanceProp); | ||
// const decorators = []; |
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.
Removable?
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.
Removed
306cd80
to
dc9df39
Compare
dc9df39
to
3374ba9
Compare
ember-decorators
Introduced a new class EOProp.js, a wrapper for
EmberObjectProperty
to avoid mutating theClassProperty
object returned from jscodeshift parser