Fragment arguments/variables (syntax/validation/execution)

[JoviDeCroock]

This spec contains amendments to #1010, a diffed view is available at mjmahone#3.

These amendments are made from comments on the implementation PR and alterations from the new implementation

• Changing fragment-key to just be fragment-name Fragment Arguments Reference Implementation graphql-js#3835 (comment)
• Removing the substitution logic and going closer to the coerce logic we use in the general flow

In general the biggest changes are that we introduce fragmentVariableValues which will be on the groupedFieldSet, these are derived from the arguments in scope of the fragmentDefinition where this field is used.

We introduce localFragmentVariables which as we are traversing down fragment-spreads are a coerced set of variables i.e.

query($b: String!, $c: String!) {
  field3(b: $b) ## uses b from variable-values
  ...A(a: "A", b: "B")
}

fragment A($a: String!, $b: String!) on X {
  field(a: $a) ## uses $a from localVariableValues
  field2(c: $c) ## we have access to variableValues so we take c from there
  ...B(b: $b) ## uses $b from localVariableValues
}

fragment B($b: String!) on X {
  ## This fragment has access to $b and $c but not to $a
  field4(b: $b) ## uses $b from localVariableValues
}

Last but not least we introduce getArgumentValuesFromSpread which looks at the spread and fragment-definition and establishes a coerced set of localVariableValues.

---

• Fixes Parameterized fragments #204

[netlify]

✅ Deploy Preview for graphql-spec-draft ready!

Name	Link
🔨 Latest commit	ef8aaa3
🔍 Latest deploy log	https://app.netlify.com/projects/graphql-spec-draft/deploys/692980a0bbe76a00087ea4bd
😎 Deploy Preview	https://deploy-preview-1081--graphql-spec-draft.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[mjmahone]

This is clear to me :)

Very excited to see progress, and thank you for cleaning up and clarifying the PR!

[JoviDeCroock]

@graphql/tsc is there anything I can do to move this forward?

[benjie]

It feels like section 2 should comment on the fact that shadowed variables may not be referenced implicitly by child fragment spreads. (This is probably not the right way of saying that!) E.g. in the following query, Forbidden is spread inside of F and attempts to use variable $a which is ambiguous, and hence not permitted.

query Q($a: Int!, $b: Int!) {
  ...F(a: 7)
}
fragment F($a: Int!) {
  ...Fine
  ...Forbidden
}
fragment Fine {
  b: echo(input: $b)
}
fragment Forbidden {
  a: echo(input: $a)
}

[JoviDeCroock]

@benjie from a purely technical perspective I agree, I was approaching that more from a backwards compatibility perspective. The Fragments variables are only applicable within the context of a FragmentDefinitiion so we don't carry nested values. A fragment is always looked at in isolation and not from a call-stack point of view.

i.e.

query ($a: String!) {
  operation {
    ...fields(a: 'some-value')
  }
}

fragment fields($a: String) on Type {
  field(x: $a) ## this carries 'some-value'
  ...moreFields
}

fragment moreFields on Type {
  field2(x: $a) ## this would carry the value passed into the operation
}

This also makes Fragment-Arguments easier to reason about for me atleast as it's either the Variables passed into the definition or from the operation itself.

[benjie]

I haven't got as far as section 6 yet.

[benjie]

@JoviDeCroock Ah! I thought we landed on the other side with this one, this does simplify things significantly because we can look at fragments in isolation 👍

[benjie]

Great work Jovi! Here's a few comments, but this was only a quick review so it's not exhaustive.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: benjie / name: Benjie (0bcdd92, 185bbac, 20fb2f7, 276230f, 4805a96, 6536944, 6659416, 77c44e2, 7ec40df, b025653, c9cb79d, d4bf325, ed12965, fa905f1)
• ✅ login: JoviDeCroock / name: Jovi De Croock (ef8aaa3)

[benjie]

@JoviDeCroock did you rebase on a different computer? Often this happens when git is configured to use a different email address on one computer than another, so some commits don't match EasyCLA whilst others do.

[JoviDeCroock]

@benjie I did not, the commits it points at are code-suggestions I applied so maybe it's not good at the co-authored ones? Not sure 😅 can rebase to fix it

[benjie]

This is an excellent start, I've gone through and done a full review of sections 2 and 5 and applied a number of editorial changes and clarifications to the text to align it more with the conventions of the spec and ensure that any ambiguities or edge cases are clarified; since there was a lot of edits I've raised them as a separate pull request:

• JoviDeCroock#2

I've not yet fully reviewed section 6, but I see some of the edits there may overlap with the incremental delivery work (that needed similar changes to the collect fields algorithm) so it would be worth making sure that these align. @robrichard and I can probably work through that and make it consistent.

Thanks for your work on this!

[martinbonnin]

Apollo Kotlin 5.0.0-alpha.5+ supports fragment arguments (PR)

[JoviDeCroock]

Thanks everyone for the discussion and work on this PR.

I won’t be able to continue championing or spending time on this proposal. If there’s still interest in moving this forward, it would likely benefit from someone else picking it up and driving it.

Appreciate all the input so far, and happy to see it continue if someone wants to take it further.

[benjie]

Thanks for your work on this, @JoviDeCroock! I've re-opened it in #1224 to ensure the work isn't lost since it's RFC2; I've marked it as "needs champion". It already feels like it's fairly close to the finish line thanks to your hard work!