Unify EntityStore#{get,getFieldValue} methods. #5772
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When I originally wrote this code, I was motivated to keep the get and getFieldValue methods separate because they had different return types. Method signature overloading solves that problem using just the type system, so we can have one method implementation rather than two. At the same time, I was able to eliminate unnecessary inheritance and method overriding in the Layer subclass by using an instanceof check in EntityStore#get.
benjamn
added a commit
that referenced
this pull request
Jan 23, 2020
One of the most important internal changes in Apollo Client 3.0 is that the result caching system now tracks its dependencies at the field level, rather than at the level of whole entity objects: #5617 As proof that this transformation is complete, we can finally remove the ability to read entire entity objects from the EntityStore by ID, so that the only way to read from the store is by providing both an ID and the name of a field. The logic I improved in #5772 is now even simpler, without the need for overloading multiple EntityStore#get signatures. In the process, I noticed that the EntityStore#has(ID) method was accidentally depending on any changes to the contents of the identified entity, even though store.has only cares about the existence of the ID in the store. This was only a problem if we called store.has during a larger read operation, which currently only happens when InMemoryCache#read is called with options.rootId. The symptoms of this oversight went unnoticed because the caching of readFragment results was just a little more sensitive to changes to the given entity. The results themselves were still logically correct, but their caching was not as effective as it could be. That's the beauty and the curse of caching: you might not notice when it isn't working, because all your tests still pass, and nobody complains that their application is broken. Fortunately, we can model this dependency with an ID plus a fake field name called "__exists", which is only dirtied when the ID is newly added to or removed from the store.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When I originally wrote this code, I was motivated to keep the
getandgetFieldValuemethods separate because they had different return types. Method signature overloading solves that problem using just the type system, so we can have one method implementation rather than two.At the same time, I was able to eliminate unnecessary inheritance and method overriding in the
Layersubclass by using aninstanceofcheck inEntityStore#get.