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

feat: execution for fragment arguments syntax #2

Merged

Conversation

JoviDeCroock
Copy link
Owner

@JoviDeCroock JoviDeCroock commented Jan 27, 2024

This implements execution of Fragment Arguments, and more specifically collecting-fields and replacing the variables in a fragment-definition with values coming from the fragment-spread, as described by the spec changes in graphql/graphql-spec#1010.

This is a 2024 update of graphql#3835 where all discussions have been carried over in code comments, they are highlighted already as review comments here as well.

This PR splits out the execution part so we can look at that in detail, I will pause here as the fragment-keying remarks will most likely have quite an impact on the validation changes.

Copy link

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 JoviDeCroock force-pushed the fragment-args-execution-2024 branch 3 times, most recently from 7ffcd86 to dde52d7 Compare January 27, 2024 10:51
@JoviDeCroock JoviDeCroock changed the title implement execution for fragment arguments syntax feat: execution for fragment arguments syntax Jan 27, 2024
@JoviDeCroock JoviDeCroock force-pushed the fragment-args-execution-2024 branch 3 times, most recently from af53dbd to 507e0dd Compare January 27, 2024 12:04
@mjmahone
Copy link

Yeah I realized (after I started) that we do need to preserve a two-layer “stack” of variables, a la the refactored context you’re describing. Substitution doesn’t cut it: I can’t remember all the details for WHY substitution doesn’t work (sounds like you’re learning them though!).

Yes it would be a pretty large refactor, but I think that refactor is essentially required to make fragment arguments good.

@yaacovCR
Copy link
Collaborator

Overall I wonder if it would be nice to have some common GraphQLArgument/GraphQLSignature type similar to #7 that could unify some of the above logic.

@yaacovCR
Copy link
Collaborator

If it helps at all, I have rebased this branch on current main: https://github.com/yaacovCR/graphql-executor/tree/fragment-args-execution-2024-rebased

@yaacovCR
Copy link
Collaborator

Here is a commit that implements some parallel changes to what I suggested in the TypeInfo branch with precompiling the fragment variable signatures....

yaacovCR@e5744fc

Also includes greater code reuse of getArgumentValues, but presently requires some JavaScript specific changes around non-provided variables, previously they did not appear at all in the args array, now they need to be present but undefined, so the code path for supplying the default field value takes precedence over the operation default value

@yaacovCR
Copy link
Collaborator

but presently requires some JavaScript specific changes around non-provided variables, previously they did not appear at all in the args array, now they need to be present but undefined

I was able to fix this on my scratch brainstorming branch: https://github.com/yaacovCR/graphql-executor/tree/execution-suggestions

@JoviDeCroock JoviDeCroock force-pushed the fragment-args-syntax-2024 branch 3 times, most recently from 839284d to 3918bb5 Compare August 20, 2024 14:44
@yaacovCR
Copy link
Collaborator

yaacovCR commented Aug 21, 2024

TODO: rebase last 1 commit on fragments args syntax

branch https://github.com/yaacovCR/graphql-executor/tree/execution-suggestions has that commit rebased on main and then one additional commit showing you one potential set of changes that would be responsive to the feedback i am trying to give.

Basically, in that branch:

  1. we reuse getArgumentValues for field arguments and fragment arguments
  2. we do not coalesce fragmentVariablesValues and regular variableValues into a single map, but just pass around both to the relevant functions
  3. we have an experimental prefix for the function that changes signature (experimentalGetArgumentValues()), everything else just gets optional arguments
  4. we do not store fragmentVariables in the context
  5. we have a separate type GraphQLVariableSignature that we use whenever we encounter a variable (this last item actually fixes a trivial bug with one-of)

@JoviDeCroock JoviDeCroock force-pushed the fragment-args-execution-2024 branch 2 times, most recently from 1d24995 to 47ff0fb Compare August 25, 2024 11:50
@JoviDeCroock JoviDeCroock force-pushed the fragment-args-execution-2024 branch 3 times, most recently from 5a2cba0 to 9f60aa8 Compare August 30, 2024 08:19
JoviDeCroock and others added 3 commits August 30, 2024 10:20
* 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
as it cannot be used by validation, which must collect all errors rather than fail with invalid type for signature
@JoviDeCroock JoviDeCroock merged commit f9a1a69 into fragment-args-syntax-2024 Sep 4, 2024
28 of 29 checks passed
JoviDeCroock added a commit to graphql/graphql-js that referenced this pull request Sep 6, 2024
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](JoviDeCroock#2)
- [TypeInfo & validation
changes](JoviDeCroock#4)
- [validation changes in
isolation](JoviDeCroock#5)


CC @mjmahone the original author

---------

Co-authored-by: mjmahone <[email protected]>
Co-authored-by: Yaacov Rydzinski <[email protected]>
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.

4 participants