-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
introduce ExecuteGroupedFieldSet, CollectRootFields and CollectSubfields #999
Conversation
✅ Deploy Preview for graphql-spec-draft ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
4fc5b32
to
b015e9c
Compare
f9c92a4
to
e9f9e9a
Compare
review suggestions merged, but now depends on #1008 to fix formatting |
Additional use case for this PR is for graphql/graphql-js#3820 ...where we merge groupedFieldSets of TaggedFieldNodes, so we can't just use CollectFields(...MergeSelectionSets(...selectionSets)...) we need an independent CollectSubFields that operates of a taggedFieldNode group... |
e9f9e9a
to
aa2ccb3
Compare
In particular, this is used as the base for robrichard#10 |
7ee2644
to
81f2f5f
Compare
fdcce17
to
b94f733
Compare
Rather than merging subSelectionSets of a field set using MergeSelectionSets and then calling CollectFields, introducing CollectSubfields allows the field set's groupedSubfieldSet to be calculated directly. This may be helpful if the specification were ever to be altered such that additional state beyond the current selection set were to be required to calculate the response, i.e. if it were to be required to know the originating selectionSet of a given field within the fieldSet for determining when to communicate a reference signal. See graphql#998 (comment)
b94f733
to
ec44866
Compare
Remove unnecessary return value change Co-authored-by: Benjie <[email protected]>
also used as the base for #1026 |
I needed something similar for my incremental delivery changes; thought you might be interested to see how I solved this problem: https://github.com/graphql/graphql-spec/compare/benjie/incremental-common?w=1 |
That looks very clean, if you want to submit it as an alternative to this PR, I would definitely +1 it :) |
Closing in favor of #1039 |
Rather than merging subSelectionSets of a field set using MergeSelectionSets and then calling CollectFields, this PR introduces CollectSubfields allows the field set's groupedSubfieldSet to be calculated directly.
The reference implementation already uses this algorithm so that this change actually aligns the specification to the reference implementation, and is ipso facto non-breaking.
Motivation: reformulating the specification in this manner may be helpful if the specification were ever to be altered such that additional state beyond the current selection set were to be required to calculate the response, i.e. if it were to be required to know the originating selectionSet of a given field within the fieldSet for determining when to communicate a reference signal. In such a scenario, it may still be quite possible to merge the set of requested data from a field set's subSelectionSets, but it may not be possible to express that merged data as an equivalent selectionSet.
In particular, currently:
For the given set of fields:
These can currently be trivially merged as:
However, the requested information for
a
in:cannot be contained in a merged selection set under A, because some of those fields will be related to Ref1 and some will not. The requsted information can still be merged, but it cannot be expressed in selection set format.