Skip to content
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

Replace ExecuteSelectionSet with ExecuteGroupedFieldSet #1039

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

benjie
Copy link
Member

@benjie benjie commented Aug 21, 2023

Essentially this PR raises the concept of "grouped field sets" to be a top-level concept, and replaces the ExecuteSelectionSet method with ExecuteGroupedFieldSet (which essentially just drops the first line of ExecuteSelectionSet, which was responsible for producing the grouped field set to be executed). It then refactors the rest of the spec to accommodate this change, reducing the repetition in ExecuteQuery, ExecuteMutation and ExecuteSubscriptionEvent; and removing MergeSelectionSets (which generated a "virtual" selection set to accomodate the ExecuteSelectionSet method), instead adding a CollectSubfields algorithm which generates 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 @defer and @stream directives - 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 the CollectSubfields algorithm 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.

@netlify
Copy link

netlify bot commented Aug 21, 2023

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit 60a9c35
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/6710d44e5c51d800083722f6
😎 Deploy Preview https://deploy-preview-1039--graphql-spec-draft.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

- For each {field} in {fields}:
- Let {fieldSelectionSet} be the selection set of {field}.
- If {fieldSelectionSet} is null or empty, continue to the next field.
- Append all selections in {fieldSelectionSet} to {selectionSet}.
- Return {selectionSet}.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should call this fieldGroupedFieldSet rather than subGroupedFieldSet as when comparing here between the non prefixed groupedFieldSet and this prefixed grouped field set, they both refer to subfields, just one is from a single node and one is the merged result.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps childGroupedFieldSet? fieldGroupedFieldSet sounds to me like a "field-grouped field set" rather than a "field's grouped-field-set".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, child has the same problem, but maybe if we change fieldSelectionSet in tandem that would work anyway through some context signaling

@benjie
Copy link
Member Author

benjie commented Feb 5, 2024

(Rebased on main)

@benjie
Copy link
Member Author

benjie commented Sep 19, 2024

(Rebased on main)

@yaacovCR
Copy link
Contributor

The only issue I noticed when working on incremental delivery, which depends on this, is the ordering of where the collect fields algorithm shows up — as it is now still in the original place, but it is used much earlier. This gets more complicated with incremental delivery as the incremental delivery plan building and execution has the same issue which exacerbates the ordering conundrum.

Might we consider moving collect fields earlier or into its own section as part of this PR?

@benjie
Copy link
Member Author

benjie commented Sep 19, 2024

@yaacovCR Just to check I understand, you mean specifically reordering the sections in specification but keeping the algorithm steps the same, rather than reordering the steps within the algorithms, right?

Maybe you'd like to make a PR to this branch which does your proposed re-arrange without any extraneous edits?

@yaacovCR
Copy link
Contributor

#1111

as just one option.

With incremental delivery, the Field Collection subsection may be supplemented by additional incremental-delivery specific subsections

@robrichard

@Keweiqu
Copy link
Contributor

Keweiqu commented Oct 3, 2024

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants