feat: add experimental support for parsing fragment arguments

[JoviDeCroock]

This is a rebase of #3847

This implements execution of Fragment Arguments, and more specifically visiting, parsing and printing of fragment-spreads with arguments and fragment definitions with variables, as described by the spec changes in graphql/graphql-spec#1081. There are a few amendments in terms of execution and keying the fragment-spreads, these are reflected in mjmahone/graphql-spec#3

The purpose is to be able to independently review all the moving parts, the stacked PR's will contain mentions of open feedback that was present at the time.

• execution changes
• TypeInfo & validation changes
• validation changes in isolation

CC @mjmahone the original author

[netlify]

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Name	Link
🔨 Latest commit	5a16135
🔍 Latest deploy log	https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/66d888218ec45f00089701e1
😎 Deploy Preview	https://deploy-preview-4015--compassionate-pike-271cb3.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 site configuration.

[mjmahone]

Thank you for rebasing the syntax changes! This looks correct to me.

I'm not sure whether we need the full stack of changes up before merging any of the changes: I'd be surprised if the syntax changes need to be modified due to the validation or execution changes.

This update is probably worth bringing to the graphql-wg this week: https://github.com/graphql/graphql-wg/blob/main/agendas/2024/02-Feb/01-wg-primary.md

[github-actions]

Hi @JoviDeCroock, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

• @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
• @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

[JoviDeCroock]

@mjmahone worth bringing up that the PR is up, or should that be for the graphql-js wg? Not entirely sure 😅

[JoviDeCroock]

CC @graphql/graphql-js-reviewers

[kitten]

The implementation in this branch passes tests but uses UNSET. However, there's an alternative here: JoviDeCroock@1ee3f42, which has some updated tests.

Basically, the spec proposal uses CoerceArgumentValues (which itself is used in CoerceFieldArgumentValues, which isn't reflected in graphql-js since the spec conflates two different data structures), which is in turn used in ArgumentsFromSpread.

The spec then uses versions of spreadArgumentValues / argumentValues, i.e. a version of “variables” only for the fragment variable definitions, and keeps the variableValues separate. Since CoerceArgumentValues is reused, we should raise an error in CollectFields (via the former) for invalid values (e.g. undefined on non-defaulting non-null fields), which is reflected in the linked commit.

[mjmahone]

worth bringing up that the PR is up, or should that be for the graphql-js wg?

I think it's worth adding a 5-10 minute dicussion item to the WG meeting this thursday https://github.com/graphql/graphql-wg/blob/main/agendas/2024/02-Feb/01-wg-primary.md

[mjmahone]

The implementation in this branch passes tests but uses UNSET. However, there's an alternative here: JoviDeCroock@1ee3f42, which has some updated tests.

I don't think that is true anymore? I think @JoviDeCroock removed all the UNSET hacks from this PR (they might still exist in the executor changes). It also looks like graphql-js #4015 no longer uses the UNSET hack.

We should definitely ship the graphql-js changes that most closely match the changes to the spec itself, and the UNSET hack does not match the spec changes from Spec #1081 , but this looks like the parser changes are in a reasonably good state right now to my eyes!

[JoviDeCroock]

@mjmahone yeah it doesn't use it it's just the parser. The execution PR also uses the new spec text

[JoviDeCroock]

@graphql/graphql-js-reviewers is there anything missing to move this and the linked PR's forward?

[nandorojo]

So excited for this! Let me know if you want any help testing it in a real app.

[JoviDeCroock]

Would this PR be better served being aimed at the 16.x.x branch? Not sure how the release schedule of this would work. If so how do we handle the existing option this._options.allowLegacyFragmentVariables

[yaacovCR]

Submitted some suggestions to the TypeInfo/Validation portion over at JoviDeCroock#12 => While working through that and thinking about the linked original review comments (https://github.com/graphql/graphql-js/pull/3835/files#r1101008869), I am tending to think we should make Fragment Arguments a separate Kind.

Basically, with my suggestions as the starting off point, anyone who has a Kind.ARGUMENT visitor in place could be visiting a field/directive argument or a fragment spread argument. You can distinguish by calling context.getArgument() which will return the argument definition for field/directive arguments, otherwise undefined, or context.getFragmentSignature() which will return the fragment signature if in a fragment spread argument, otherwise undefined.

Code within custom rules that do not know about fragment arguments will interpret the undefined result from context.getArgument() as a missing argument definition.

In the original version, we monkey-patch a schema argument definition from the fragment spread argument, so the argument will not appear missing, but then the visitor doesn't really know what it's visiting, and may actually do the wrong thing.

There does not yet appear to me to be a great backwards compatible fix for this, and so I suggest a separate Kind.FRAGMENT_ARGUMENT.

[yaacovCR]

Motivation is only to preserve backwards compatibility for those not wishing to upgrade their visitors while fragment arguments are experimental. So another alternative is something in between, introduce a new Node now, maybe something like Kind.EXPERIMENTAL_FRAGMENT_ARGUMENT and then when fragment arguments are released, we would remove the EXPERIMENTAL and force everyone to upgrade their existing stuff (and force early adopters to integrate any custom visitors for both types of arguments).