Refactor custom field merge function processing. #5712
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
Ever since implementing support for custom merge functions, I have been unsatisfied with the MergeOverrides concept, especially because a mistake in the processing of overrides recently resulted in a fairly significant bug, fixed by commit 8c58be5. Instead of tracking MergeOverrides separately from the fields processed by processSelectionSet and processFieldValue (which is not only error-prone but also complicates the return values of those methods), we can embed the necessary FieldNode and __typename information within the field data returned by processSelectionSet, using recognizeable FieldValueToBeMerged objects, which are safely removed by Policies#applyMerges before the fully processed fields are written into the EntityStore (which knows nothing about custom merge functions or FieldValueToBeMerged objects). One small benefit of this refactoring is that we can call store.getFieldValue(dataId, name) instead of store.get(dataId) in Policies#applyMerges, which brings us a little closer to eliminating EntityStore#get once and for all.
benjamn
commented
Dec 19, 2019
hwillson
approved these changes
Dec 20, 2019
|
@hwillson Thanks for the review! I'm also pretty happy with the refactoring commit that I just pushed, in case you want to take a look (not vital). |
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.
Ever since implementing support for custom
mergefunctions, I have been unsatisfied with theMergeOverridesconcept, especially because a mistake in the processing of overrides recently resulted in a fairly significant bug, fixed by commit 8c58be5.Instead of tracking
MergeOverridesseparately from the fields processed byprocessSelectionSetandprocessFieldValue(which is not only error-prone but also complicates the return values of those methods), we can embed the necessaryFieldNodeand__typenameinformation within the field data returned byprocessSelectionSet, using recognizableFieldValueToBeMergedobjects, which are safely removed byPolicies#applyMergesbefore the fully processed fields are written into theEntityStore(which knows nothing about custommergefunctions orFieldValueToBeMergedobjects).One small benefit of this refactoring is that we can call
store.getFieldValue(dataId, name)instead ofstore.get(dataId)inPolicies#applyMerges, which brings us a little closer to eliminatingEntityStore#getonce and for all.