Merge incoming fragment fields before running merge functions and updating the EntityStore
#8372
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
Fixes #8370 by postponing all context.store.merge updates until after all processSelectionSet work is done, performing the store updates at the end of StoreWriter#writeToStore, whereas previously those updates happened at the end of each StoreWriter#processSelectionSet call. The postponement of store updates gives the StoreWriter a chance to merge together multiple partial objects representing the same logical object (possibly matched by different fragments along different query paths), before running merge functions with applyMerges. Previously, those partial objects would be merged separately into context.store, so later objects might replace earlier objects in cases where the objects are unidentifiable/non-normalized, and there is no merge function (or merge:true) to force the objects to merge. Of course, you could configure merge:true to explicily permit merging unidentified objects with a given __typename. Configuring merge:true for the 'Container' type seems to fixes the reproduction in #8370. But this explicit configuration should not be necessary when we know the objects in question represent the same logical object, because they were derived from the same original result object (same ID, not necessarily ===). In other words, this PR should wipe out a whole class of situations where we used to show "Cache data may be lost..." warnings, since we can now safely merge the fields of unidentifiable objects that share the same original result object, without requiring an explicit merge:true configuration to permit that mixing of fields.
benjamn
force-pushed
the
8370-fix-fragment-merging
branch
from
3ba39b6 to
4935d11
Compare
benjamn
changed the title
merge functions and updating the EntityStore
benjamn
commented
Jun 11, 2021
Comment on lines
+1405
to
+1412
| it('correctly merges fragment fields along multiple paths', () => { | ||
| const cache = new InMemoryCache({ | ||
| typePolicies: { | ||
| Container: { | ||
| // Uncommenting this line fixes the test, but should not be necessary, | ||
| // since the Container response object in question has the same | ||
| // identity along both paths. | ||
| // merge: true, |
There was a problem hiding this comment.
As I wrote in #8370 (comment), this merge: true configuration was going to be my answer to issue #8370, before I realized that it shouldn't be necessary (hence this PR).
brainkim
reviewed
Jun 14, 2021
brainkim
reviewed
Jun 14, 2021
brainkim
approved these changes
Jun 14, 2021
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.
Fixes #8370 by postponing all
context.store.mergeupdates until after allprocessSelectionSetwork is done, performing the store updates at the end ofStoreWriter#writeToStore, whereas previously those updates happened at the end of eachStoreWriter#processSelectionSetcall.The postponement of store updates gives the
StoreWritera chance to merge together multiple partial objects representing the same logical object (possibly matched by different fragments along different query paths), before runningmergefunctions withapplyMerges.Previously, those partial objects would be merged separately into
context.store, so later objects might replace earlier objects in cases where the objects are unidentifiable/non-normalized, and there was nomergefunction (ormerge: trueshorthand) to force the objects to merge.Of course, you could configure
merge: trueto explicitly permit merging unidentified objects with a given__typename. Configuringmerge: truefor theContainertype seems to fixes the reproduction in #8370. But this explicit configuration should not be necessary when we know the objects in question represent the same logical object, because they were derived from the same incoming result object (same ID, not necessarily===objects).In other words, this PR should wipe out a whole class of situations where we used to show
Cache data may be lost...warnings, since we can now safely merge the fields of unidentifiable objects that share the same original result object, without requiring an explicitmerge: trueconfiguration to permit that mixing of fields.