Replace ExecuteSelectionSet with ExecuteCollectedFields#1039
Merged
Conversation
✅ Deploy Preview for graphql-spec-draft ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This was referenced Aug 21, 2023
yaacovCR
reviewed
Aug 21, 2023
robrichard
approved these changes
Aug 21, 2023
benjie
force-pushed
the
benjie/incremental-common
branch
from
4d62b8b to
b342b58
Compare
Member
Author
|
(Rebased on |
benjie
force-pushed
the
benjie/incremental-common
branch
from
b342b58 to
a52310e
Compare
Member
Author
|
(Rebased on |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
JoviDeCroock
approved these changes
Oct 3, 2024
Contributor
|
@benjie we need to formally define "grouped field set" in Section 2 "Language". You might have done so in a prior PR that I missed. Here is the link to us defining "section sets" in section 2 https://spec.graphql.org/draft/#sec-Selection-Sets |
JoviDeCroock
added a commit
to JoviDeCroock/graphql-spec
that referenced
this pull request
Feb 15, 2025
benjie
force-pushed
the
benjie/incremental-common
branch
from
6e76340 to
3c6dfb3
Compare
benjie
commented
May 1, 2025
Comment on lines
+398
to
+404
| :: A _field set_ is a list of selected fields that share the same _response | ||
| name_ (the field alias if defined, otherwise the field's name). | ||
|
|
||
| Note: The order of field selections in a _field set_ is significant, hence the | ||
| algorithms in this specification model it as a list. Any later duplicated field | ||
| selections in a field set will not impact its interpretation, so using an | ||
| ordered set would yield equivalent results. |
Member
Author
There was a problem hiding this comment.
martinbonnin
reviewed
May 1, 2025
| ``` | ||
|
|
||
| Valid operations must supply a nested field set for any field that returns an | ||
| Valid operations must supply a selection of fields for any field that returns an |
Contributor
There was a problem hiding this comment.
Not directly related to this PR but a more precise wording would be to use selectionSet here as fragments are also valid, not only fields.
Suggested change
| Valid operations must supply a selection of fields for any field that returns an | |
| Valid operations must supply a valid sub _selection set_ for any field that returns an |
martinbonnin
approved these changes
May 1, 2025
leebyron
force-pushed
the
benjie/incremental-common
branch
from
1e42cc4 to
d0fb75c
Compare
leebyron
force-pushed
the
benjie/incremental-common
branch
from
ed11e66 to
9c4a529
Compare
Collaborator
|
@benjie I made some substantial editorial changes here, especially to one of the term names. Please review! |
leebyron
force-pushed
the
benjie/incremental-common
branch
from
e4a5199 to
c776fa7
Compare
leebyron
force-pushed
the
benjie/incremental-common
branch
from
c776fa7 to
97d43ba
Compare
github-project-automation
Bot
moved this from In Progress
to Done
in Benjie's GraphQL tasks
Member
Author
|
Thanks Lee! Suggested edits raised in #1175 |
leebyron
pushed a commit
that referenced
this pull request
Jul 2, 2025
* Resolve ambiguity - we mean return type
For `type User { name: String }`, `User.name` is a field of an object
type (`User`). Clarify that we mean the return type of the field, not
the type to which it belongs.
* Consistency with collectedFieldsMap
* Reword to avoid 'During execution, ... before execution.'
* Serial execution relates to the set of fields, not each individual field
* Add missing close parenthesis
* Remove duplicate 'by', specific algorithm is detailed in next paragraph
* CollectFields() produces many _field set_
* Minor edits
23 tasks
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
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.
Essentially this PR replaces the
ExecuteSelectionSetmethod withExecuteCollectedFields(which essentially just drops the first line ofExecuteSelectionSet, which was responsible for collecting fields to be executed). It then refactors the rest of the spec to accommodate this change, reducing the repetition inExecuteQuery,ExecuteMutationandExecuteSubscriptionEvent; and removingMergeSelectionSets(which generated a "virtual" selection set to accomodate theExecuteSelectionSetmethod), instead adding aCollectSubfieldsalgorithm which produces a grouped field set directly, ready for execution.I extracted this common refactoring from a number of my attempts to write spec changes for the
@deferand@streamdirectives - it turns out that this refactoring of the spec was always needed as a base for my changes. Similarly, @yaacovCR found similar in his attempts to address this same problem, and raised #999 extracted from his solution. This PR was introduced independently of #999 (other than using theCollectSubfieldsalgorithm name) however there is significant alignment, so @yaacovCR suggested that I raise it as an alternative PR.It may be easier to review this PR in "split" view rather than "unified" view.