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

introduce ExecuteGroupedFieldSet, CollectRootFields and CollectSubfields #999

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

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented Nov 6, 2022

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:

{
  a {
    subfield1
  }
  ...ExampleFragment
}

fragment ExampleFragment on Query {
  a {
    subfield2
  }
  b
}

For the given set of fields:

a {
  subfield1
}
a {
  subfield2
}

These can currently be trivially merged as:

a {
  subfield1
  subfield2
}

However, the requested information for a in:

{
  a {
    subfield1
  }
  Ref1 { completed } : ...ExampleFragment
}

fragment ExampleFragment on Query {
  a {
    subfield2
  }
  b
}

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.

@netlify
Copy link

netlify bot commented Nov 6, 2022

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit 6f1ad74
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/644aa92284b3ba00082c0f30
😎 Deploy Preview https://deploy-preview-999--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 settings.

@yaacovCR yaacovCR force-pushed the CollectSubfields branch 3 times, most recently from 4fc5b32 to b015e9c Compare November 11, 2022 09:43
spec/Section 6 -- Execution.md Outdated Show resolved Hide resolved
spec/Section 6 -- Execution.md Outdated Show resolved Hide resolved
spec/Section 6 -- Execution.md Outdated Show resolved Hide resolved
@yaacovCR yaacovCR force-pushed the CollectSubfields branch 2 times, most recently from f9c92a4 to e9f9e9a Compare January 13, 2023 08:50
@yaacovCR
Copy link
Contributor Author

review suggestions merged, but now depends on #1008 to fix formatting

@yaacovCR
Copy link
Contributor Author

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...

@yaacovCR
Copy link
Contributor Author

In particular, this is used as the base for robrichard#10

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)
@yaacovCR yaacovCR changed the title introduce CollectRootFields and CollectSubfields introduce ExecuteGroupedFieldSet, CollectRootFields and CollectSubfields Mar 20, 2023
Remove unnecessary return value change

Co-authored-by: Benjie <benjie@jemjie.com>
@yaacovCR
Copy link
Contributor Author

also used as the base for #1026

@michaelstaib michaelstaib added the 💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md) label Jun 1, 2023
@benjie
Copy link
Member

benjie commented Jul 8, 2023

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

@yaacovCR
Copy link
Contributor Author

yaacovCR commented Jul 9, 2023

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 :)

@benjie
Copy link
Member

benjie commented Aug 21, 2023

@yaacovCR Thanks; I have now done so: #1039

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants