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
Fragment args 2024 amendments #1081
base: main
Are you sure you want to change the base?
Fragment args 2024 amendments #1081
Conversation
✅ Deploy Preview for graphql-spec-draft ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is clear to me :)
Very excited to see progress, and thank you for cleaning up and clarifying the PR!
spec/Section 5 -- Validation.md
Outdated
fragment potentiallyConflictingArguments( | ||
$commandOne: DogCommand! | ||
$commandTwo: DogCommand! | ||
) on Dog { | ||
...commandFragment(command: $commandOne) | ||
...commandFragment(command: $commandTwo) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh I had the thought "we should just always make this invalid" but realized that's not really any different from making it invalid to have a fragment spread anywhere with a different variable name as the argument.
This being allowed is a weird consequence of how fragment arguments compose. We maybe should disallow spreading the same fragment at the same level within the same fragment but different argument values (or variables), but only in the exact same way that we disallow field usage with different arguments on the same level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's good to allow for ...commandFragment ...commandFragment
(two argumentless fragment-spreads) for backwards compatability and ...commandFragment(a: $a) ...commandFragment(a: $a)
because they are the same which in my mind ties intuitively into the former case while the different arguments should be disallowed for ease of reasoning otherwise we'd have to go into the fragment-spreads and analyse whether this changed variable results in conflicting fields due to arguments on the field or @include/@skip
/...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Matt - this should use the same logic as field usage. This would thus be invalid for the same reason that this query (and a query using any two of these three fields) would be invalid:
query ($id1: ID! = "ZmlsbXM6MQ==", $id2: ID! = "ZmlsbXM6MQ==") {
node(id: "ZmlsbXM6MQ==") { id }
node(id: $id1) { id }
node(id: $id2) { id }
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made it more clear in 03ba255
spec/Section 5 -- Validation.md
Outdated
@@ -1563,12 +1636,36 @@ fragment HouseTrainedFragment on Query { | |||
} | |||
``` | |||
|
|||
Likewise, it is valid for both an operation and a fragment to define a variable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe say "for a fragment to define a variable with a name that is also defined on an operation" to disambiguate that we don't mean fragments and operations can define two of the same variable within a single definition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 48fa841, does that sound better?
|
||
**Formal Specification** | ||
|
||
- For every {operation} in the document: | ||
- Let {variables} be the variables defined by that {operation}. | ||
- Each {variable} in {variables} must be used at least once in either the | ||
operation scope itself or any fragment transitively referenced by that | ||
operation. | ||
operation, excluding fragments that define the same name as an argument. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah this is a very good catch
spec/Section 2 -- Language.md
Outdated
Variables can be used within fragments. Operation-defined variables have global | ||
scope with a given operation, so a variable used within a fragment must either | ||
be declared in any top-level operation that transitively consumes that fragment, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sentence would flow better if we do something like this --
Operation-defined variables have... Fragment-defined variables have ... So a variable used within a fragment must ...
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds great, what do you think about 5a73543
spec/Section 5 -- Validation.md
Outdated
- Let {spreadsForName} be the set of fragment spreads with a given name in | ||
{visitedSelections}. | ||
- Given each pair of members {spreadA} and {spreadB} in {spreadsForName}: | ||
- {spreadA} and {spreadB} must have identical sets of arguments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we specify "must have identical sets of arugments and values?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and directives?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually hadn't thought about directives, that's a great point! Addressed in 48fa841
Co-authored-by: Matt Mahoney <mahoney.mattj@gmail.com>
5a73543
to
d7590fa
Compare
@graphql/tsc is there anything I can do to move this forward? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
}
Co-authored-by: Benjie <benjie@jemjie.com>
@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 i.e.
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. |
ac9fdbc
to
03ba255
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't got as far as section 6 yet.
spec/Section 5 -- Validation.md
Outdated
- Let {visitedSelections} be the selections in {set} including visiting | ||
fragments and inline fragments and applying any supplied fragment spread | ||
arguments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence is doing a lot. Maybe an algorithm would be more appropriate here? Also note that "applying arguments" is under-defined, in particular because validation does not have access to variable values.
- Let {spreadsForName} be the set of fragment spreads with a given name in | ||
{visitedSelections}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is doing a lot; I would prefer to clarify it with an algorithm:
- Let {spreadsForName} be {GroupFragmentSpreadsByName(visitedSelections)}.
...
GroupFragmentSpreadsByName(selections):
- Let {groupedFragmentSpreads} be a map.
- For each {selection} in {selections}:
- If {selection} is a {FragmentSpread}:
- Let {name} be the name of {selection}.
- Let {fragmentSpreadsWithName} be the set in {groupedFragmentSpreads} for {name}; if no such set exists, create it as an empty set.
- Add {selection} to {fragmentSpreadsWithName}.
- Return {groups}.
but I see this is based on existing spec text which does similar for fields, so I guess this isn't needed.
@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 👍 |
Co-authored-by: Benjie <benjie@jemjie.com>
Co-authored-by: Benjie <benjie@jemjie.com>
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
coerce
logic we use in the general flowIn general the biggest changes are that we introduce
fragmentVariableValues
which will be on thegroupedFieldSet
, 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.Last but not least we introduce
getArgumentValuesFromSpread
which looks at the spread and fragment-definition and establishes a coerced set of localVariableValues.