Validate changeset getter #54
Conversation
If we need to validate based on derived property, not the raw changes the only place where we can put a getter to calculate the value of that property is the changeset itself as it buffers all changes. This test case illustrates the failure of that scenario.
If there is a validation based on derived property we should get its value in _valueFor().
It seems that the [ERRORS] object is used here as a "cache" for the value. In fact it doesn't give us much benefit as it is not called multiple times during a single validation. Using the cache for derived property doesn't work because it is not properly discarded when any relevant properties are changed. It seems reasonable to remove the usage of cache to avoid potential issues and confusions.
| let content: Content = this[CONTENT]; | ||
|
|
||
| if (errors.hasOwnProperty(key)) { | ||
| let e: Err = errors[key]; | ||
| return e.value; |
There was a problem hiding this comment.
What are your thoughts on removing this?
There was a problem hiding this comment.
I've put it in a separate commit with my thoughts.
Basically I think that this "caching" is redundant here and can be error-prone.
I initially put the logic of taking the property after that section and it didn't work because the cache wasn't invalidated when related fields were changed. So, I've put it before that block and then removed this if section in a separate to get your feedback about that.
If you think it is useful, I can just drop the last commit.
There was a problem hiding this comment.
Gotcha! Looks like we don't have a test for this case. If we try and validate a key, is it possible the validation results only lives in ERRORS? Perhaps we can add a test case to check this assumption?
There was a problem hiding this comment.
The code I removed just accesses the value, contained in ERRORS object, not the validation results. The value can be always accessed from the changes or the underlying object. Therefore I consider this thing as a performance optimization, not the necessary functionality.
Maybe I'm missing something?
In any case, this change is not really tied to the original issue I'm trying to address. I can extract it to a separate PR or just drop it.
What do you think about the change in 30c0b2a?
There was a problem hiding this comment.
Separate PR may be good unless we can confirm with a test. The changes you have for the specific issue seem 💯
There was a problem hiding this comment.
value can be always accessed from the changes or the underlying object
That is - a test confirming that adding an error will result in the right validations. (perhaps we already have one)
There was a problem hiding this comment.
If you mean addError API, there is a bunch of tests tagged with #addError.
If we need to validate based on derived property, not the raw changes
the only place where we can put a getter to calculate the value of that
property is the changeset itself as it buffers all changes.
This PR is an attempt to make it work.