Support cache.identify(entity) for computing entity ID strings. #5642
Conversation
After you've specified accurate keyFields policies for your types, the last thing you want is for application code to reimplement that logic just to compute an ID to use with methods like InMemoryCache#evict. This commit introduces a method called InMemoryCache#identify, which takes a result object and computes its ID based on its __typename and primary key fields, so that you can do cache.evict(cache.identify(entity)) to evict an object from the cache, without any manual string manipulation.
| const rootSelectionSet = getOperationDefinition(query).selectionSet; | ||
| const abcsSelectionSet = (rootSelectionSet.selections[0] as FieldNode).selectionSet; | ||
| const fragmentMap = createFragmentMap(getFragmentDefinitions(query)); | ||
|
|
||
| expect(cache.identify(object, abcsSelectionSet, fragmentMap)).toBe( |
There was a problem hiding this comment.
As I was typing out this part of the test, I found myself wondering whether anyone will ever go to the trouble of passing a SelectionSet and a FragmentMap as additional arguments to cache.identify. Those arguments are important for the underlying policies.identify method, since we do have the selection set available when we're writing results to the cache, and it's important to handle field aliasing correctly, but I wonder if we should limit this public API to accepting just the StoreObject, and assume that its fields have not been renamed by aliasing? For what it's worth, cache.identify will throw if any of the keyFields are missing (as I'm testing just above).
There was a problem hiding this comment.
@benjamn I'm okay with either approach, but leaving it like this does add extra flexibility.
There was a problem hiding this comment.
@benjamn I'm in favor of only having the StoreObject as it is the piece they will have readily available within their application. Passing in additional selectionSet/fragmentMap would require keeping additional state about the graph accessible in places where I don't think it should really be found
There was a problem hiding this comment.
I think a picture of where/how this will be used when using aliases to evict data would be helpful to know if it is useable without the selection set part
There was a problem hiding this comment.
Yeah, the alternative is just to tell folks to make sure they're not renaming any of the primary key fields in the object they pass to cache.identify. Even in the rare cases where someone actually does need to rename an aliased field in a result object before passing it to cache.identify, that's a lot easier to do by hand in a few places than finding the relevant selection set and computing a fragment map. I'm comfortable telling people that the reason their normalization is broken is because of aliasing. Also, once they start using keyFields, cache.identify will throw if any of the necessary fields are missing, so it shouldn't be that much of a mystery.
benjamn
changed the title
| const rootSelectionSet = getOperationDefinition(query).selectionSet; | ||
| const abcsSelectionSet = (rootSelectionSet.selections[0] as FieldNode).selectionSet; | ||
| const fragmentMap = createFragmentMap(getFragmentDefinitions(query)); | ||
|
|
||
| expect(cache.identify(object, abcsSelectionSet, fragmentMap)).toBe( |
There was a problem hiding this comment.
@benjamn I'm okay with either approach, but leaving it like this does add extra flexibility.
| const rootSelectionSet = getOperationDefinition(query).selectionSet; | ||
| const abcsSelectionSet = (rootSelectionSet.selections[0] as FieldNode).selectionSet; | ||
| const fragmentMap = createFragmentMap(getFragmentDefinitions(query)); | ||
|
|
||
| expect(cache.identify(object, abcsSelectionSet, fragmentMap)).toBe( |
There was a problem hiding this comment.
@benjamn I'm in favor of only having the StoreObject as it is the piece they will have readily available within their application. Passing in additional selectionSet/fragmentMap would require keeping additional state about the graph accessible in places where I don't think it should really be found
| const rootSelectionSet = getOperationDefinition(query).selectionSet; | ||
| const abcsSelectionSet = (rootSelectionSet.selections[0] as FieldNode).selectionSet; | ||
| const fragmentMap = createFragmentMap(getFragmentDefinitions(query)); | ||
|
|
||
| expect(cache.identify(object, abcsSelectionSet, fragmentMap)).toBe( |
There was a problem hiding this comment.
I think a picture of where/how this will be used when using aliases to evict data would be helpful to know if it is useable without the selection set part
After you've specified accurate
keyFieldspolicies for your types, the last thing you want is for application code to reimplement that logic just to compute IDs to use with methods likeInMemoryCache#evict.This commit introduces a method called
InMemoryCache#identify, which takes a result object and computes its ID based on its__typenameand primary key fields, so that you can docache.evict(cache.identify(entity))to evict an object from the cache, without any manual string manipulation.