refactor: reduce amount of work on initialization #88
Conversation
addon/-private/wrap-descriptor.js
Outdated
| set(newValue) { | ||
| initialized = true; | ||
|
|
||
| runValidator(validator, this.constructor, key, newValue, 'set'); |
There was a problem hiding this comment.
One thing I'm realizing is that unless we call super, the last class that defines the argument will always win:
class Foo {
@argument('string') baz;
}
class Bar extends Foo {
@argument('number') baz;
}We may need to store all of the validators in an array with addValidationFor and then run all of them in the setter.
There was a problem hiding this comment.
I'm not sure I'm following -- Are the usage in Foo and Bar related, even though the classes are independent?
There was a problem hiding this comment.
yes sorry, that was a typo, corrected
There was a problem hiding this comment.
Gotcha. We'd talked about this briefly earlier -- we have a few options for defining arguments for the same property on multiple components in a hierarchy
- First one (lowest in the chain) is run
- Last one (highest in the chain) is run
- All of them are run
We currently do the first option there, but I'm happy to refactor to run all of them. That should be pretty easy now with our and combinator!
|
|
||
| wrapComputedProperty(constructor, this, meta, validation, key); | ||
|
|
||
| runValidation(validation, constructor, key, this[key], 'init'); |
There was a problem hiding this comment.
I'm not sure we need to run validations here, since class fields and getters/setters will trigger when the values are set anyways. I suppose the init part of the error message is still somewhat valuable, but it probably won't show up anyways since the error for the accessor will trigger first.
There was a problem hiding this comment.
That would remove the init validations altogether, right?
I'm fine with that, personally -- I think it probably makes more sense just to run the validations on get/set
There was a problem hiding this comment.
So, I looked into this and I'm not sure that it'll make sense to remove the init validation.
I tried removing the validator on init and adding a validator to get. This had the side-effect of making it impossible to check that the props passed to the object match the expected validation, because as part of initializing the object in .create(), the initial value is accessed. Basically, the validation on get will be called before the property is overwritten with the new value, which doesn't work.
| @@ -26,7 +26,7 @@ module('Integration | Component Behavior', function(hooks) { | |||
|
|
|||
There was a problem hiding this comment.
We should probably add some tests that cover inheritance behavior, to make sure that it works correctly.
There was a problem hiding this comment.
The tests in tests/unit/argument-test.js should cover the inheritance use-case -- there are a number of examples there around extending classes with @argument defined in a base class and overwritten (or not) in a parent class.
alexlafroscia
force-pushed
the
better-property-wrapping
branch
from
ea045f6 to
db4fcbd
Compare
|
^ Rebased to make compatible with the conflicting changes in #90 |
- Simplify approach to wrapping most properties - Reduce the amount of work done when initializing objects
It wasn't previously clear that the `wrapComputedProperty` behavior is only invoked for computed properties. This code functions the same, but makes it easier to see that the computed property wrapping is only sometimes performed
alexlafroscia
force-pushed
the
better-property-wrapping
branch
from
db4fcbd to
c670ce8
Compare
|
New |
This is a follow up to some of the feedback given in #85