-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix: @attr defaultValue() results should persist after initialization #9355
Fix: @attr defaultValue() results should persist after initialization #9355
Conversation
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 believe we also need to clear defaultAttrs during patchLocalAttributes
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.
Thanks for the quick feedback!
I reviewed patchLocalAttributes
and happy to make the change, but feel like I need a bit more clarification. FWICT, the purpose of that method is to simply remove the key/value pairs from localAttrs
that are not in remoteAttrs
or inflightAttrs
. I'm assuming this would occur when, say, a record is updated from the server while there are still local changes? If so, should we also have similar logic to remove any defaultAttrs
that have keys present in localAttrs
, remoteAttrs
, or inflightAttrs
?
The way I see it, yes it might be worthwhile to ensure defaultAttrs
is kept clean at every possible chance, but because the order of operations of getAttr
would inherently prevent a default value from being exposed if any of the other sets of attributes contains that key, it could be considered superfluous.
In thinking through the scenario, if I understand it correctly:
- A record is fetched remotely that does not define an attribute with a
defaultValue
function, so the default value is created. - The record is returned from the server again, only this time it does have a value for that, thereby masking the previous default value.
- The record is returned from the server, yet without the attribute once again. What should happen?
This seems like quite an edge case, but I think my opinion would be that the original default value would take effect again, as opposed to a new one being created.
Happy to do what you think makes the most sense here, just let me know!
Also, I was hoping to find a nice set of unit tests for this cache class, but no luck 😕. Do you have a recommendation regarding where to write some tests for this?
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 seems like quite an edge case, but I think my opinion would be that the original default value would take effect again, as opposed to a new one being created.
If the server has previously informed us of a state, and that state later disappears, imo the correct thing to do is start over.
I was hoping to find a nice set of unit tests for this cache class, but no luck
The cache requires integration tests. Newest ones are here https://github.com/emberjs/data/tree/main/tests/ember-data__json-api/tests/integration/cache and here https://github.com/emberjs/data/blob/main/tests/main/tests/integration/cache/json-api-cache-test.js
but most of them are scattered through the main test app, with most having been written from the perspective of user observable behaviors (due largely to how cache grew out of Store/Model/InternalModel).
I'd recommend adding new tests at that first link above since this is a behavior specific to JSON:API cache.
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.
@christophersansone got any more time for this one? I can poke at it a bit if needed
Sorry! The day job got in the way this week. Planning to spend some time on it this weekend. |
@runspired Sorry for the delay! Okay, I added the change to I did not figure out how to adequately test the change for Let me know if there's anything else that makes sense to test. |
@runspired Is this something you think could be easily backported into 4.12? |
@daniellubovich the backport commit shouldn't be too hard |
…emberjs#9355) * Backport of emberjs#9355 * defaultValue function results should persist after initialization * clear default values on patch changes * defaultValue caching test --------- Co-authored-by: christophersansone <chris@voltage.io>
…9441) * Fix: @attr defaultValue() results should persist after initialization (#9355) * Backport of #9355 * defaultValue function results should persist after initialization * clear default values on patch changes * defaultValue caching test --------- Co-authored-by: christophersansone <chris@voltage.io> * Fix lint errors --------- Co-authored-by: Christopher Sansone <chris@legendary.io> Co-authored-by: christophersansone <chris@voltage.io> Co-authored-by: Daniel Lubovich <dlubovich@cradlepoint.com>
Description
Fixes #9315.
(cc @runspired)
Notes for the release
Before this change, when
@attr
defines adefaultValue
as a function, the default value instantiated on a model instance will not necessarily be the value that is serialized. If the attribute was notset
, and therefore the default value is used, then the serialization would call thedefaultValue
function again instead of returning the value on the model. This is mostly a problem when the attribute value is an array or object, because changes can be made to the object, resulting in those changes being lost upon serialization. With this change, the originaldefaultValue
is persisted at the cache level to avoid this issue.