Skip to content

Add fragment-arguments#6882

Merged
martinbonnin merged 4 commits into
mainfrom
fragment-arguments
Feb 24, 2026
Merged

Add fragment-arguments#6882
martinbonnin merged 4 commits into
mainfrom
fragment-arguments

Conversation

@martinbonnin
Copy link
Copy Markdown
Contributor

@martinbonnin martinbonnin commented Feb 24, 2026

This is based on graphql/graphql-spec#1081

Open questions:

  • Do we need a new directive location (FRAGMENT_VARIABLE_DEFINITION)? This PR doesn't.
  • Do we need to validate fragment spreads have the exact same arguments? This PR currently doesn't and instead substitutes the fragment variables in the fields and runs the default field merging validation.

No answer is needed at this point. The feature is behind a feature flag in the parser.

Note: once a document is successfully parsed, there's no feature flag anymore, everything else supports fragment arguments experimentally.

@martinbonnin martinbonnin requested a review from BoD as a code owner February 24, 2026 13:08
@apollo-librarian
Copy link
Copy Markdown

apollo-librarian Bot commented Feb 24, 2026

✅ Docs preview ready

The preview is ready to be viewed. View the preview

File Changes

0 new, 2 changed, 0 removed
* (developer-tools)/kotlin/v5/migration/5.0.mdx
* (developer-tools)/kotlin/v5/testing/data-builders.mdx

Build ID: 05d00e2f8255adb76aa46e40
Build Logs: View logs

URL: https://www.apollographql.com/docs/deploy-preview/05d00e2f8255adb76aa46e40


✅ AI Style Review — No Changes Detected

No MDX files were changed in this pull request.

Review Log: View detailed log

This review is AI-generated. Please use common sense when accepting these suggestions, as they may not always be accurate or appropriate for your specific context.

Copy link
Copy Markdown
Contributor

@BoD BoD left a comment

Choose a reason for hiding this comment

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

Nice!!! 🎉

Love how this doesn't impact codegen.

@BoD
Copy link
Copy Markdown
Contributor

BoD commented Feb 24, 2026

Do we need to validate fragment spreads have the exact same arguments?

Hmm I'm not following, do you have an example?

…pollo/ast/internal/ExecutableValidationScope.kt

Co-authored-by: Benoit 'BoD' Lubek <BoD@JRAF.org>
@martinbonnin
Copy link
Copy Markdown
Contributor Author

martinbonnin commented Feb 24, 2026

Hmm I'm not following, do you have an example?

There is this text in the spec PR:

- Given each pair of distinct members {spreadA} and {spreadB} in
  {spreadsForFragment}:
  - {spreadA} and {spreadB} must have identical sets of arguments.

This makes sense because this is not valid:

fragment queryDetails($arg: Int!) on Query {
  foo(arg: $arg)
}

{
  ...queryDetails(arg: 0)
  ...queryDetails(arg: 1)
}

But we need to validate against non spreads as well:

{
  ...queryDetails(arg: 0)
  ...queryDetails(arg: 1)
  foo(arg: 2)
}

So what I'm doing is I remove the variables before validation:

{
  foo(arg: 0)
  foo(arg: 1)
  foo(arg: 2) 
}

And I just run the field merging algorithm. There's no part that tries to check fragments specifically.

In other terms, the message will always be field at path "foo" cannot be merged... and not fragment spread "queryDetails" cannot be merged.

I think that's OK.

In a world where fragment definitions can only use the fragment-defined variables, this would simplify the validation. But since we can't really do this, might as well run our good old validation.

@BoD
Copy link
Copy Markdown
Contributor

BoD commented Feb 24, 2026

Ah I see, thanks! So adding this validation would fail earlier with a more specific message. Makes sense an agree it's fine as-is 👍

@martinbonnin martinbonnin merged commit 4a43a85 into main Feb 24, 2026
7 checks passed
@martinbonnin martinbonnin deleted the fragment-arguments branch February 24, 2026 17:56
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.

2 participants