Change "grouped field set" to be a map of ordered sets #1161
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.
Extension to #1039
Currently:
I propose that we defined "field set" to be an ordered set (rather than list) of fields that all share the same response name. This makes a lot of the spec clearer, e.g. it's much clearer why
ExecuteField()
should receive a "field set" (since they all share the same response name) rather than simply receiving "fields".Essentially I propose:
This also makes the term "grouped field set" a lot clearer.
It also increases efficiency; consider this query:
Prior to this change:
ExecuteRootSelectionSet
usesCollectFields
to produce this grouped field set:CompleteValue
callsCollectSubfields
to produce this grouped field set (assumingCat
):visitedFragments
doesn't prevent this because it doesn't work across sibling selections, only nested selectionsIf we were to have deeper nesting, this duplication would get much worse. Memory wise, this isn't a big deal, but looping-wise we're looping a lot more than we need to.
By explicitly making this a set, we remove this duplication. The phrase "if not already present" makes it clear that the first field selection "wins", which is important for ordering.