Fragment Arguments: Spec Implementation #1010
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for graphql-spec-draft ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
This was referenced Jan 19, 2023
mjmahone
force-pushed
the
fragment-args-2023
branch
from
9dbac33
to
bcca1ba
Compare
This was referenced Jan 27, 2024
JoviDeCroock
reviewed
Feb 1, 2024
Comment on lines
+520
to
+525
| - Let {arguments} be the set of arguments on {selection}. | ||
| - Let {fragmentSpreadKey} be a unique key of {fragmentSpreadName} and | ||
| {arguments}. | ||
| - If {fragmentSpreadKey} is in {visitedFragments}, continue with the next | ||
| {selection} in {selectionSet}. | ||
| - Add {fragmentSpreadKey} to {visitedFragments}. |
There was a problem hiding this comment.
Judging from graphql/graphql-js#3835 (comment) this isn't needed, or am I misunderstanding that?
Suggested change
| - Let {arguments} be the set of arguments on {selection}. | |
| - Let {fragmentSpreadKey} be a unique key of {fragmentSpreadName} and | |
| {arguments}. | |
| - If {fragmentSpreadKey} is in {visitedFragments}, continue with the next | |
| {selection} in {selectionSet}. | |
| - Add {fragmentSpreadKey} to {visitedFragments}. | |
| - If {fragmentSpreadName} is in {visitedFragments}, continue with the next | |
| {selection} in {selectionSet}. | |
| - Add {fragmentSpreadName} to {visitedFragments}. |
JoviDeCroock
reviewed
Feb 1, 2024
Comment on lines
+529
to
+532
| - Let {fragmentWithArgumentSubstitutions} be the result of calling | ||
| {SubstituteFragmentArguments(fragment, arguments)}. | ||
| - Let {fragmentSelectionSet} be the top-level selection set of | ||
| {fragmentWithArgumentSubstitutions}. |
There was a problem hiding this comment.
We changed this to create a shadowed copy of variables that is relevant to the fragment-spread we are on
JoviDeCroock
reviewed
Feb 1, 2024
| @@ -562,9 +568,36 @@ DoesFragmentTypeApply(objectType, fragmentType): | |||
| - if {objectType} is a possible type of {fragmentType}, return {true} | |||
| otherwise return {false}. | |||
|
|
|||
| SubstituteFragmentArguments(fragment, arguments): | |||
There was a problem hiding this comment.
We'll need to replace this with getArgumentValuesFromSpread https://github.com/JoviDeCroock/graphql-js/pull/2/files#diff-777202fe667b9e656f40631c341c7c0776895a75eff7004578bf9b7a97e095abR161
|
Amendments from my implementation are in mjmahone#3 |
|
I second Benjie's move that we replace this by #1081, which has substantial improvements (and will make merging easier, when there aren't stacked PRs). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fully specifies the Fragment Arguments changes required, as evidenced by the implementation changes made to
graphql-jsin graphql/graphql-js#3835This is an update to a years-old RFC PR #865, but given that PR had a lot of discussion that has been resolved, it should be easier for reviewers to start with this PR as a cleaner slate. I've tried to incorporate all discussion from previous PRs and Working Group discussion into the initial commit here.
Please use this PR for reviewing and verifying the accuracy of the PR, with inline comments when possible.
For discussions about Fragment Arguments as an idea, missing features or open questions, please open or respond to a thread in the graphql-wg discussion space: graphql/graphql-wg#1239