feat: add experimental support for parsing fragment arguments #4015
Conversation
✅ Deploy Preview for compassionate-pike-271cb3 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
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
|
Hi @JoviDeCroock, I'm @github-actions bot happy to help you with this PR 👋 Supported commandsPlease post this commands in separate comments and only one per comment:
|
|
@mjmahone worth bringing up that the PR is up, or should that be for the graphql-js wg? Not entirely sure 😅 |
|
CC @graphql/graphql-js-reviewers |
|
The implementation in this branch passes tests but uses Basically, the spec proposal uses The spec then uses versions of |
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 |
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! |
|
@mjmahone yeah it doesn't use it it's just the parser. The execution PR also uses the new spec text |
|
@graphql/graphql-js-reviewers is there anything missing to move this and the linked PR's forward? |
|
So excited for this! Let me know if you want any help testing it in a real app. |
|
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 |
JoviDeCroock
force-pushed
the
fragment-args-syntax-2024
branch
3 times, most recently
from
839284d
to
3918bb5
Compare
Co-authored-by: mjmahone <[email protected]>
|
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 Basically, with my suggestions as the starting off point, anyone who has a Code within custom rules that do not know about fragment arguments will interpret the undefined result from 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 |
|
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 |
at least for now!
* implement execution for fragment arguments syntax Co-authored-by: mjmahone <[email protected]> * add directive test (#9) * add directive test * add failing test add additional nested fragment test (#8) Correct test and lint stuff suggestions for execution (#11) * introduce internal getVariableSignature utility now extracted also to graphql-js PR, see graphql#4175 * execution suggestions fixes execution to always use fragment variable when has the same name as an operation variable previously, we were allowing an operation variable to be used if the fragment variable was not provided, and the field had no default. Now, we still use the fragment variable, and so the value is null. this now correct logic allows us to significantly reduce the diff from main adds additional test * move getVariableSignature to execution as it cannot be used by validation, which must collect all errors rather than fail with invalid type for signature --------- Co-authored-by: mjmahone <[email protected]> Co-authored-by: Yaacov Rydzinski <[email protected]>
* Add typeinfo functionality as a run-up to supporting the new validation rules Co-authored-by: mjmahone <[email protected]> * Fragment arguments validation (#5) * Fragment args validation * add experimental substitution * validation suggestions (#12) * validation suggestions * remove OperationSignature from ValidationContext only used in one rule, does not need to be on context remove dependency on getVariableSignature move FRAGMENT_ARGUMENT below ARGUMENT fragment arguments do not have location defaults only variable defaults => so getDefaultValue(), which returns location defaults for use with the allowedVariableUsage helper, should never be called by getVariableUsages with respect to fragment arguments fragment arguments therefore need not add anything to the default value stack reduce diff from main these diffs crept in before Kind.FRAGMENT_ARGUMENT was separated out --------- Co-authored-by: Yaacov Rydzinski <[email protected]> * OverlappingFieldsCanBeMergedRule suggestions (#15) --------- Co-authored-by: mjmahone <[email protected]> Co-authored-by: Yaacov Rydzinski <[email protected]>
latest edits not included within graphql#4015
this was actually intended to be in graphql#4015, but due to branch confusion not originally included now we also have a test
by creating a map for each fragment with fragment variables with the properly combined local and global variables background: this was in the original PR graphql#4015, was taken out by me when i misunderstood and thought that when a fragment variable shadows an operation variable, the operation variable could still leak through if the fragment variable had no default, and I thought it was necessary within getArgumentValues to still have the signatures advantages to the original approach, which this PR restores: does not change the signature of getVariableValues, getDirectiveValues, valueFromAST disadvantages: creating a combined variable map of local and global variables may be a waste if the global variables are not actually used in that fragment
by creating a map for each fragment with fragment variables with the properly combined local and global variables background: this was in the original PR graphql#4015, was taken out by me when i misunderstood and thought that when a fragment variable shadows an operation variable, the operation variable could still leak through if the fragment variable had no default, and I thought it was necessary within getArgumentValues to still have the signatures advantages to the original approach, which this PR restores: does not change the signature of getVariableValues, getDirectiveValues, valueFromAST disadvantages: creating a combined variable map of local and global variables may be a waste if the global variables are not actually used in that fragment
by creating a map for each fragment with fragment variables with the properly combined local and global variables background: this was in the original PR graphql#4015, was taken out by me when i misunderstood and thought that when a fragment variable shadows an operation variable, the operation variable could still leak through if the fragment variable had no default, and I thought it was necessary within getArgumentValues to still have the signatures advantages to the original approach, which this PR restores: does not change the signature of getVariableValues, getDirectiveValues, valueFromAST disadvantages: creating a combined variable map of local and global variables may be a waste if the global variables are not actually used in that fragment
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.
CC @mjmahone the original author