From 16be477477f7d4ad01ea15e503cac72ef16eacce Mon Sep 17 00:00:00 2001 From: mjmahone Date: Mon, 2 Jan 2023 17:45:26 -0500 Subject: [PATCH 1/7] Fragment Arguments added to spec --- rfcs/FragmentArguments.md | 351 ++++++++++++++++++++++++++ spec/Appendix B -- Grammar Summary.md | 12 +- spec/Section 2 -- Language.md | 79 +++++- spec/Section 3 -- Type System.md | 1 + spec/Section 4 -- Introspection.md | 2 + spec/Section 5 -- Validation.md | 153 ++++++++++- spec/Section 6 -- Execution.md | 76 ++++-- 7 files changed, 624 insertions(+), 50 deletions(-) create mode 100644 rfcs/FragmentArguments.md diff --git a/rfcs/FragmentArguments.md b/rfcs/FragmentArguments.md new file mode 100644 index 000000000..f48e4d80e --- /dev/null +++ b/rfcs/FragmentArguments.md @@ -0,0 +1,351 @@ +## RFC: Fragment Arguments + +# Problem: Variable Modularity + +GraphQL fragments are designed to allow a client's data requirements to compose. +Two different screens can use the same underlying UI component. If that +component has a corresponding fragment, then each of those screens can include +exactly the data required by having each query spread the child component's +fragment. + +This modularity begins to break down for variables. As an example, let's imagine +a FriendsList component that shows a variable number of friends. We would have a +fragment for that component like so: + +``` +fragment FriendsList on User { + friends(first: $nFriends) { + name + } +} +``` + +In one use, we might want to show some screen-supplied number of friends, and in +another the top 10. For example: + +``` +fragment AnySizedFriendsList on User { + name + ...FriendsList +} + +fragment TopFriendsUserProfile on User { + name + profile_picture { uri } + ...FriendsList +} +``` + +Even though every usage of `TopFriendsUserProfile` should be setting `$nFriends` +to `10`, the only way to enforce that is by manually walking all callers of +`TopFriendsUserProfile`, recursively, until you arrive at the operation +definition and verify the variable is defined like `$nFriends: Int = 10`. + +This causes a few major usability problems: + +- If I ever want to change the number of items `TopFriendsUserProfile` includes, + I now need to update every _operation_ that includes it. This could be dozens + or hundreds of individual locations. +- Even if the component for `TopFriendsUserProfile` is only able to display 10 + friends, in most clients at runtime the user can override the default value, + enabling a mismatch between the data required and the data asked for. + +# Existing Solution: Relay's `@arguments`/`@argumentDefinitions` + +Relay has a solution for this problem by using a custom, non-spec-compliant pair +of directives, +[`@arguments`/`@argumentDefinitions`](https://relay.dev/docs/api-reference/graphql-and-directives/#arguments). + +These directives live only in user-facing GraphQL definitions, and are compiled +away prior to making a server request. + +Following the above example, if we were using Relay we'd be able to write: + +``` +fragment FriendsList on User @argumentDefinitions(nFriends: {type: "Int!"}) { + friends(first: $nFriends) { + name + } +} + +fragment AnySizedFriendsList on User { + name + ...FriendsList @arguments(nFriends: $operationProvidedFriendCount) +} + +fragment TopFriendsUserProfile on User { + name + profile_picture { uri } + ...FriendsList @arguments(nFriends: 10) +} +``` + +Before sending a query to the server, Relay compiles away these directives so +the server, when running an operation using `TopFriendsUserProfile`, sees: + +``` +fragment FriendsList on User { + friends(first: 10) { + name + } +} + +fragment TopFriendsUserProfile on User { + name + profile_picture { uri } + ...FriendsList +} +``` + +The exact mechanics of how Relay rewrites the user-written operation based on +`@arguments` supplied is not the focus of this RFC. + +However, even to enable this client-compile-time operation transformation, Relay +had to introduce _non-compliant directives_: each argument to `@arguments` +changes based on the fragment the directive is applied to. While syntactically +valid, this fails the +[Argument Names validation](https://spec.graphql.org/draft/#sec-Argument-Names). + +Additionally, the `@argumentDefinitions` directive gets very verbose and unsafe, +using strings to represent variable type declarations. + +Relay has supported `@arguments` in its current form since +[v2.0](https://github.com/facebook/relay/releases/tag/v2.0.0), released in +January 2019. There's now a large body of evidence that allowing fragments to +define arguments that can be passed into fragment spreads is a significant +usability improvement, and valuable to the wider GraphQL community. However, if +we are to introduce this notion more broadly, we should make sure the ergonomics +of it conform to users' expectations. + +# Proposal: Introduce Fragment Argument Definitions, which allow using arguments on Fragment Spreads + +Relay's `@arguments`/`@argumentDefinitions` concepts provide value, and can be +applied against GraphQL written for existing GraphQL servers so long as there is +a pre-server compiler which transforms the concept away. + +## New Fragment Argument Definition syntax + +For the `@argumentDefinitions` concept, we can allow fragments to share the same +syntax as operation level definitions. Going back to the previous example, this +would look like: + +``` +fragment FriendsList($nFriends: Int!) on User { + friends(first: $nFriends) { + name + } +} +``` + +The syntax re-uses the concepts from Variable Definitions, so that when you +_define_ and _use_ the argument, it preserves the same appearance (`$` + name). + +## New Fragment Spread Argument syntax + +For the `@arguments` concept, we can allow fragment spreads to share the same +syntax as field and directive arguments. + +``` +fragment AnySizedFriendsList on User { + name + ...FriendsList(nFriends: $operationProvidedFriendCount) +} + +fragment TopFriendsUserProfile on User { + name + profile_picture { uri } + ...FriendsList(nFriends: 10) +} +``` + +This may feel a little weird: for fields, arguments are defined as +`argName: Type` and then used like `...Foo(argName: $variable)`. The +alternatives here are: + +- Define `argName: Type` for fragment arguments + - This has the disadvantage of seeing both the argument definition and the + argument usage in the same fragment with different styles. +- Call `...Foo($argName: $variable)` + - This feels incredibly confusing: `$` typically means "replace this token + with a value", and that's not what's happening. + +Notably, this proposed syntax, of using `$name` at the definition and usage +site, and `name:` when calling the Fragment/Function, is the convention that PHP +uses for +[Named Arguments](https://www.php.net/manual/en/functions.arguments.php#functions.named-arguments). +Given GraphQL was designed with many PHP-isms, it seems like we should re-use +the conventions chosen there when there's no clear reason not to. + +## Scope: Local + +Fragment Arguments should always have local scope. This gets us closer to the +idea that while operations are "global", fragments behave more like well-scoped +functions. + +This has a bunch of beneficial side effects: + +- For validation, we don't need ot check for fragment argument definition + clashes +- For composability, I can use the same argument on many fragments and not worry + about unrelated fragments. +- For ease-of-understanding, you don't need to keep track of how all child + fragments use a fragment argument to understand how changing something like + the default value will modify the results. +- Makes it easy to update Variables In Allowed Positions, as we don't need to + hunt the definition of a variable across many potential parent fragments. + +The other scoping options are: + +- Global, i.e. a fragment argument is just syntactic sugar for an operation + variable. + - This is what we implemented at Meta, and it was terrible for all the reasons + you can think of. +- Recursively local, i.e. the variable takes on any parent fragment argument + definition + - This preserves the concept of the value being some sort of recursively + scoped variable. + - However, as explained above, keeping track of what's happening, and + preventing fragment conflicts, becomes really difficult. + +We're choosing to explicitly _allow_ overriding operation variables, as the +local scope means you can clearly see whether a variable is scoped to the +fragment or operation. + +## New Validation Rule: Fragment Argument Definitions Used in Fragment + +With local scope, this rule is very simple. + +``` +fragment Foo($x: Int) on User { + name +} +``` + +would be invalid. + +Additionally, + +``` +fragment Foo($x: Int!) on User { + ...Bar +} + +fragment Bar { + number(x: $x) +} +``` + +would also be invalid: even though `$x` is used underneath Foo, it is used +outside of Foo's explicit definition. In this context, `$x` in Bar is actually +an operation variable. + +However, this would be valid: + +``` +fragment Foo($x: Int!) on User { + ...Bar(x: $x) +} + +fragment Bar($x: Int) { + number(x: $x) +} +``` + +### Consideration: how strict should this rule be? + +As an initial RFC, I'd advocate for encouraging the _strictest_ version of this +rule possible: any argument defined on a fragment must be explicitly used by +that same fragment. It would be easy to relax the rule later, but very difficult +to do the reverse. + +It's clearly more composable if, when changing a child fragment, you don't need +to worry about modifying argument definitions on parent fragments callsites. +However, we could in the future allow annotating argument definitions with +`@deprecated`. + +## Updated Validation Rule: Required Arguments are Provided + +We update +[Required Arguments](https://spec.graphql.org/draft/#sec-Required-Arguments) to +include fragment spreads. This makes the validation's first two bullets: + +- For each Field, **Fragment Spread** or Directive in the document. +- Let _arguments_ be the set of argument definitions of that Field, **Fragment** + or Directive. + +With this rule, the below example is invalid, even if the argument +`User.number(x:)` is nullable in the schema. + +``` +fragment Foo on User { + ...Bar +} + +fragment Bar($x: Int!) on User { + number(x: $x) +} +``` + +## Updated Validation: Overlapping Fields Can Be Merged + +Previously, fragment spreads didn't have to be considered as unique selections +in the overlapping field merging algorithm. However, the algorithm still +de-duplicated common fragment spreads. + +With this change, we can either just treat deduplicated fragment spreads as +being keyed by (name, arguments) rather than just by name, and then apply the +arguments when traversing the child selection set. + +Alternatively, and as implemented in +[graphql-js](https://github.com/graphql/graphql-js/pull/3152), we can both keep +track of the updated fragment keys and also short-circuit whenever we encounter +two fragments with the same name but different arguments. + +## WILL NOT IMPLEMENT Validation Rule: Argument Uniqueness + +If the client pre-server compiler rewrites an operation, it's possible to end up +with a selection set that violates +[Field Selection Merging](https://spec.graphql.org/draft/#sec-Field-Selection-Merging) +validation. Additionally, we have no mechanism on servers today to handle the +same fragment having different variable values depending on that fragment's +location in an operation. + +Therefore, any Fragment Spread for the same Fragment in an Operation could be +required to have non-conflicting argument values passed in. + +As an example, this is invalid: + +``` +query { + user { + best_friend { + ...UserProfile(imageSize: 100) + } + ...UserProfile(imageSize: 200) + } +} +``` + +Note: today Relay's compiler handles this ambiguity. In an extreme +simplification, this is done by producing two unique versions of `UserProfile`, +where in `UserProfile_0` `$imageSize` is replaced with `100`, and in +`UserProfile_1` `$imageSize` is replaced with `200`. However, there exist client +implementations that are unable to have multiple applications of the same +fragment within a single operation (the clients I work on cannot use Relay's +trick). + +This validation rule is more strict than necessary: the graphql-js +implementation did not require it, given the Overlapping Fields Can Be Merged +changes that protect against mis-merged fields. + +# Implementation + +This proposal is implemented completely in +[graphql-js](https://github.com/graphql/graphql-js/pull/3152) + +## Initial Implementation + +In the initial implementation, I tried to change as little as possible. This +means I only added a single new validation rule. Additionally, there may be some +weirdness around internal grammar and AST node naming/usage, but the actual +behavior should be feature complete. diff --git a/spec/Appendix B -- Grammar Summary.md b/spec/Appendix B -- Grammar Summary.md index 2291ee35f..446e9c04d 100644 --- a/spec/Appendix B -- Grammar Summary.md +++ b/spec/Appendix B -- Grammar Summary.md @@ -168,15 +168,20 @@ Arguments[Const] : ( Argument[?Const]+ ) Argument[Const] : Name : Value[?Const] -FragmentSpread : ... FragmentName Directives? +FragmentSpread : ... FragmentName Arguments? Directives? InlineFragment : ... TypeCondition? Directives? SelectionSet -FragmentDefinition : fragment FragmentName TypeCondition Directives? -SelectionSet +FragmentDefinition : fragment FragmentName FragmentArgumentsDefinition? +TypeCondition Directives? SelectionSet FragmentName : Name but not `on` +FragmentArgumentsDefinition : ( FragmentArgumentDefinition+ ) + +FragmentArgumentDefinition : Description? Variable : Type DefaultValue? +Directives[Const]? + TypeCondition : on NamedType Value[Const] : @@ -396,6 +401,7 @@ ExecutableDirectiveLocation : one of - `FRAGMENT_SPREAD` - `INLINE_FRAGMENT` - `VARIABLE_DEFINITION` +- `FRAGMENT_ARGUMENT_DEFINITION` TypeSystemDirectiveLocation : one of diff --git a/spec/Section 2 -- Language.md b/spec/Section 2 -- Language.md index 1aca650a8..1470f0b42 100644 --- a/spec/Section 2 -- Language.md +++ b/spec/Section 2 -- Language.md @@ -516,10 +516,10 @@ which returns the result: ## Fragments -FragmentSpread : ... FragmentName Directives? +FragmentSpread : ... FragmentName Arguments? Directives? -FragmentDefinition : fragment FragmentName TypeCondition Directives? -SelectionSet +FragmentDefinition : fragment FragmentName FragmentArgumentsDefinition? +TypeCondition Directives? SelectionSet FragmentName : Name but not `on` @@ -1209,13 +1209,76 @@ size `60`: **Variable Use Within Fragments** -Variables can be used within fragments. Variables have global scope with a given -operation, so a variable used within a fragment must be declared in any -top-level operation that transitively consumes that fragment. If a variable is -referenced in a fragment and is included by an operation that does not define -that variable, that operation is invalid (see +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, +or by that same fragment as a fragment argument. If a variable is referenced in +a fragment that does not define it as an argument and is included by an +operation that does not define that variable, that operation is invalid (see [All Variable Uses Defined](#sec-All-Variable-Uses-Defined)). +## Fragment Arguments + +FragmentArgumentsDefinition : ( FragmentArgumentDefinition+ ) + +FragmentArgumentDefinition : Description? Variable : Type DefaultValue? +Directives[Const]? + +Fragments may define locally scoped arguments, which can be used in locations +that accept variables. This allows fragments to be re-used while enabling the +caller to specify the fragment's behavior. + +For example, the profile picture may need to be a different size depending on +the parent context: + +```graphql example +query withFragmentArguments { + user(id: 4) { + ...dynamicProfilePic(size: 100) + friends(first: 10) { + id + name + ...dynamicProfilePic + } + } +} + +fragment dynamicProfilePic($size: Int! = 50) on User { + profilePic(size: $size) +} +``` + +In this case, the `user` will have a larger `profilePic` than those found in the +list of `friends`. + +A fragment argument is scoped to the fragment that defines it. Fragment +arguments are allowed to shadow operation variables. + +```graphql example +query withShadowedVariables($size: Int) { + user(id: 4) { + ...variableProfilePic + } + secondUser: user(id: 5) { + ...dynamicProfilePic(size: 10) + } +} + +fragment variableProfilePic on User { + ...dynamicProfilePic(size: $size) +} + +fragment dynamicProfilePic($size: Int!) on User { + profilePic(size: $size) +} +``` + +The profilePic for `user` will be determined by the variables set by the +operation, while `secondUser` will always have a profilePic of size 10. In this +case, the fragment `variableProfilePic` uses the operation defined variable, +while `dynamicProfilePic` uses the value passed in via the fragment spread's +argument `size:`. + ## Type References Type : diff --git a/spec/Section 3 -- Type System.md b/spec/Section 3 -- Type System.md index cce64b857..bc47d9f32 100644 --- a/spec/Section 3 -- Type System.md +++ b/spec/Section 3 -- Type System.md @@ -1876,6 +1876,7 @@ ExecutableDirectiveLocation : one of - `FRAGMENT_SPREAD` - `INLINE_FRAGMENT` - `VARIABLE_DEFINITION` +- `FRAGMENT_ARGUMENT_DEFINITION` TypeSystemDirectiveLocation : one of diff --git a/spec/Section 4 -- Introspection.md b/spec/Section 4 -- Introspection.md index 3054a9f6c..522b5928a 100644 --- a/spec/Section 4 -- Introspection.md +++ b/spec/Section 4 -- Introspection.md @@ -206,6 +206,7 @@ enum __DirectiveLocation { FRAGMENT_SPREAD INLINE_FRAGMENT VARIABLE_DEFINITION + FRAGMENT_ARGUMENT_DEFINITION SCHEMA SCALAR OBJECT @@ -475,6 +476,7 @@ supported. All possible locations are listed in the `__DirectiveLocation` enum: - {"FRAGMENT_SPREAD"} - {"INLINE_FRAGMENT"} - {"VARIABLE_DEFINITION"} +- {"FRAGMENT_ARGUMENT_DEFINITION"} - {"SCHEMA"} - {"SCALAR"} - {"OBJECT"} diff --git a/spec/Section 5 -- Validation.md b/spec/Section 5 -- Validation.md index dceec126b..493025aaf 100644 --- a/spec/Section 5 -- Validation.md +++ b/spec/Section 5 -- Validation.md @@ -570,6 +570,50 @@ fragment conflictingDifferingResponses on Pet { } ``` +Fragment arguments can also cause fields to fail to merge. + +While the following is valid: + +```graphql example +fragment commandFragment($command: DogCommand!) on Dog { + doesKnowCommand(dogCommand: $command) +} + +fragment potentiallyConflictingArguments( + $commandOne: DogCommand! + $commandTwo: DogCommand! +) on Dog { + ...commandFragment(command: $commandOne) + ...commandFragment(command: $commandTwo) +} + +fragment safeFragmentArguments on Dog { + ...potentiallyConflictingArguments(commandOne: SIT, commandTwo: SIT) +} +``` + +it is only valid because `safeFragmentArguments` uses +`potentiallyConflictingArguments` with the same value for `commandOne` and +`commandTwo`. Therefore `commandFragment` resolves `doesKnowCommand`'s +`dogCommand:` arg to `SIT` in both cases. + +However, by changing the argument values: + +```graphql counter-example +fragment conflictingFragmentArguments on Dog { + ...potentiallyConflictingArguments(commandOne: SIT, commandTwo: DOWN) +} +``` + +the response will have two conflicting versions of the `doesKnowCommand` that +cannot merge. + +One strategy to resolving field conflicts caused by duplicated fragment spreads +is to short-circuit when two fragment spreads with the same name are found to be +merging with different argument values. In this case, validation could short +circuit when `commandFragment(command: SIT)` is merged with +`commandFragment(command: DOWN)`. + ### Leaf Field Selections **Formal Specification** @@ -647,8 +691,8 @@ query directQueryOnObjectWithSubFields { ## Arguments -Arguments are provided to both fields and directives. The following validation -rules apply in both cases. +Arguments are provided to fields, fragment spreads and directives. The following +validation rules apply in each case. ### Argument Names @@ -657,7 +701,7 @@ rules apply in both cases. - For each {argument} in the document: - Let {argumentName} be the Name of {argument}. - Let {argumentDefinition} be the argument definition provided by the parent - field or definition named {argumentName}. + field, fragment definition or directive definition named {argumentName}. - {argumentDefinition} must exist. **Explanatory Text** @@ -675,9 +719,18 @@ fragment argOnRequiredArg on Dog { fragment argOnOptional on Dog { isHouseTrained(atOtherHomes: true) @include(if: true) } + +fragment withFragmentArg($command: DogCommand) on Dog { + doesKnowCommand(dogCommand: $command) +} + +fragment usesFragmentArg on Dog { + ...withFragmentArg(command: DOWN) +} ``` -the following is invalid since `command` is not defined on `DogCommand`. +the following is invalid since `command:` is not defined on +`Dog.doesKnowCommand`. ```graphql counter-example fragment invalidArgName on Dog { @@ -685,6 +738,15 @@ fragment invalidArgName on Dog { } ``` +and this is also invalid as `$dogCommand` is not defined on fragment +`withFragmentArg`. + +```graphql counter-example +fragment invalidFragmentArgName on Dog { + ...withFragmentArg(dogCommand: SIT) +} +``` + and this is also invalid as `unless` is not defined on `@include`. ```graphql counter-example @@ -727,9 +789,9 @@ fragment multipleArgsReverseOrder on Arguments { ### Argument Uniqueness -Fields and directives treat arguments as a mapping of argument name to value. -More than one argument with the same name in an argument set is ambiguous and -invalid. +Fields, fragment spreads and directives treat arguments as a mapping of argument +name to value. More than one argument with the same name in an argument set is +ambiguous and invalid. **Formal Specification** @@ -741,10 +803,11 @@ invalid. #### Required Arguments -- For each Field or Directive in the document: - - Let {arguments} be the arguments provided by the Field or Directive. - - Let {argumentDefinitions} be the set of argument definitions of that Field - or Directive. +- For each Field, Fragment Spread or Directive in the document: + - Let {arguments} be the arguments provided by the Field, Fragment Spread or + Directive. + - Let {argumentDefinitions} be the set of argument definitions of that Field, + Fragment Spread or Directive. - For each {argumentDefinition} in {argumentDefinitions}: - Let {type} be the expected type of {argumentDefinition}. - Let {defaultValue} be the default value of {argumentDefinition}. @@ -1776,7 +1839,7 @@ included in that operation. - 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. **Explanatory Text** @@ -1828,6 +1891,29 @@ fragment isHouseTrainedWithoutVariableFragment on Dog { } ``` +Fragment arguments can shadow operation variables: fragments that use an +argument are not using the operation defined variable of the same name. + +Likewise, it would be invalid if the variable was shadowed by a fragment +argument: + +```graphql counter-example +query variableNotUsedWithinFragment($atOtherHomes: Boolean) { + dog { + ...shadowedVariableFragment + } +} + +fragment shadowedVariableFragment($atOtherHomes: Boolean) on Dog { + isHouseTrained(atOtherHomes: $atOtherHomes) +} +``` + +because +{$atOtherHomes} is only referenced in a fragment that defines it as a +locally scoped argument, the operation defined {$atOtherHomes} +variable is never used. + All operations in a document must use all of their variables. As a result, the following document does not validate. @@ -1853,6 +1939,41 @@ fragment isHouseTrainedFragment on Dog { This document is not valid because {queryWithExtraVar} defines an extraneous variable. +### All Fragment Arguments Used + +**Formal Specification** + +- For every {fragment} in the document: + - Let {arguments} be the arguments defined by that {fragment}. + - Each {argument} in {arguments} must be used at least once in the fragment's + scope. + +**Explanatory Text** + +All arguments defined by a fragment must be used in that same fragment. Because +fragment arguments are scoped to the fragment they're defined on, if the +fragment does not contain a variable with the same name as the argument, then +the argument is superfluous. + +For example the following is invalid: + +```graphql counter-example +query queryWithFragmentArgUnused($atOtherHomes: Boolean) { + dog { + ...fragmentArgUnused(atOtherHomes: $atOtherHomes) + } +} + +fragment fragmentArgUnused($atOtherHomes: Boolean) on Dog { + isHouseTrained +} +``` + +This document is invalid because even though `fragmentArgUnused` is spread with +the argument `atOtherHomes`, and even though `$atOtherHomes` is defined as an +operation variable, there is never a variable `$atOtherHomes` used within the +scope of `fragmentArgUnused`. + ### All Variable Usages Are Allowed **Formal Specification** @@ -1861,8 +1982,12 @@ variable. - Let {variableUsages} be all usages transitively included in the {operation}. - For each {variableUsage} in {variableUsages}: - Let {variableName} be the name of {variableUsage}. - - Let {variableDefinition} be the {VariableDefinition} named {variableName} - defined within {operation}. + - If the usage is within a {fragment} that defines an argument of + {variableName}: + - Let {variableDefinition} be the {ArgumentDefinition} named + {variableName} defined within {fragment}. + - Otherwise, let {variableDefinition} be the {VariableDefinition} named + {variableName} defined within {operation}. - {IsVariableUsageAllowed(variableDefinition, variableUsage)} must be {true}. diff --git a/spec/Section 6 -- Execution.md b/spec/Section 6 -- Execution.md index 28862ea89..ee97c5cb2 100644 --- a/spec/Section 6 -- Execution.md +++ b/spec/Section 6 -- Execution.md @@ -332,10 +332,11 @@ First, the selection set is turned into a grouped field set; then, each represented field in the grouped field set produces an entry into a response map. -ExecuteSelectionSet(selectionSet, objectType, objectValue, variableValues): +ExecuteSelectionSet(selectionSet, objectType, objectValue, variableValues, +argumentValues): - Let {groupedFieldSet} be the result of {CollectFields(objectType, - selectionSet, variableValues)}. + selectionSet, variableValues, argumentValues)}. - Initialize {resultMap} to an empty ordered map. - For each {groupedFieldSet} as {responseKey} and {fields}: - Let {fieldName} be the name of the first entry in {fields}. Note: This value @@ -344,7 +345,7 @@ ExecuteSelectionSet(selectionSet, objectType, objectValue, variableValues): {objectType}. - If {fieldType} is defined: - Let {responseValue} be {ExecuteField(objectType, objectValue, fieldType, - fields, variableValues)}. + fields, variableValues, argumentValues)}. - Set {responseValue} as the value for {responseKey} in {resultMap}. - Return {resultMap}. @@ -490,7 +491,8 @@ The depth-first-search order of the field groups produced by {CollectFields()} is maintained through execution, ensuring that fields appear in the executed response in a stable and predictable order. -CollectFields(objectType, selectionSet, variableValues, visitedFragments): +CollectFields(objectType, selectionSet, variableValues, argumentValues, +visitedFragments): - If {visitedFragments} is not provided, initialize it to the empty set. - Initialize {groupedFields} to an empty ordered map of lists. @@ -513,20 +515,24 @@ CollectFields(objectType, selectionSet, variableValues, visitedFragments): - Append {selection} to the {groupForResponseKey}. - If {selection} is a {FragmentSpread}: - Let {fragmentSpreadName} be the name of {selection}. - - If {fragmentSpreadName} is in {visitedFragments}, continue with the next - {selection} in {selectionSet}. - - Add {fragmentSpreadName} to {visitedFragments}. - Let {fragment} be the Fragment in the current Document whose name is {fragmentSpreadName}. - If no such {fragment} exists, continue with the next {selection} in {selectionSet}. + - Let {spreadArgumentValues} be the result of calling + {ArgumentsFromSpread(selection, fragment, variableValues, argumentValues)} + - Let {fragmentSpreadKey} be a unique key of {fragmentSpreadName} and + {spreadArgumentValues}. + - If {fragmentSpreadKey} is in {visitedFragments}, continue with the next + {selection} in {selectionSet}. + - Add {fragmentSpreadKey} to {visitedFragments}. - Let {fragmentType} be the type condition on {fragment}. - If {DoesFragmentTypeApply(objectType, fragmentType)} is false, continue with the next {selection} in {selectionSet}. - Let {fragmentSelectionSet} be the top-level selection set of {fragment}. - Let {fragmentGroupedFieldSet} be the result of calling {CollectFields(objectType, fragmentSelectionSet, variableValues, - visitedFragments)}. + spreadArgumentValues, visitedFragments)}. - For each {fragmentGroup} in {fragmentGroupedFieldSet}: - Let {responseKey} be the response key shared by all fields in {fragmentGroup}. @@ -541,7 +547,7 @@ CollectFields(objectType, selectionSet, variableValues, visitedFragments): - Let {fragmentSelectionSet} be the top-level selection set of {selection}. - Let {fragmentGroupedFieldSet} be the result of calling {CollectFields(objectType, fragmentSelectionSet, variableValues, - visitedFragments)}. + argumentValues, visitedFragments)}. - For each {fragmentGroup} in {fragmentGroupedFieldSet}: - Let {responseKey} be the response key shared by all fields in {fragmentGroup}. @@ -550,6 +556,14 @@ CollectFields(objectType, selectionSet, variableValues, visitedFragments): - Append all items in {fragmentGroup} to {groupForResponseKey}. - Return {groupedFields}. +ArgumentsFromSpread(fragmentSpread, fragment, variableValues, +parentArgumentValues): + +- Let {argumentDefinitions} be the arguments defined on {fragment} +- Let {spreadArguments} be the arguments set on {fragmentSpread} +- Return the result of {CoerceArgumentValues(argumentDefinitions, + spreadArguments, variableValues, parentArgumentValues)} + DoesFragmentTypeApply(objectType, fragmentType): - If {fragmentType} is an Object Type: @@ -573,16 +587,17 @@ coerces any provided argument values, then resolves a value for the field, and finally completes that value either by recursively executing another selection set or coercing a scalar value. -ExecuteField(objectType, objectValue, fieldType, fields, variableValues): +ExecuteField(objectType, objectValue, fieldType, fields, variableValues, +fragmentArgumentValues): - Let {field} be the first entry in {fields}. - Let {fieldName} be the field name of {field}. -- Let {argumentValues} be the result of {CoerceArgumentValues(objectType, field, - variableValues)} +- Let {argumentValues} be the result of {CoerceFieldArgumentValues(objectType, + field, variableValues, fragmentArgumentValues)} - Let {resolvedValue} be {ResolveFieldValue(objectType, objectValue, fieldName, argumentValues)}. - Return the result of {CompleteValue(fieldType, fields, resolvedValue, - variableValues)}. + variableValues, fragmentArgumentValues)}. ### Coercing Field Arguments @@ -593,13 +608,19 @@ the type system to have a specific input type. At each argument position in an operation may be a literal {Value}, or a {Variable} to be provided at runtime. -CoerceArgumentValues(objectType, field, variableValues): +CoerceFieldArgumentValues(objectType, field, variableValues, +fragmentArgumentValues): -- Let {coercedValues} be an empty unordered Map. - Let {argumentValues} be the argument values provided in {field}. - Let {fieldName} be the name of {field}. - Let {argumentDefinitions} be the arguments defined by {objectType} for the field named {fieldName}. +- Return {CoerceArgumentValues(argumentDefinitions, argumentValues, + variableValues, fragmentArgumentValues)} + +CoerceArgumentValues(argumentDefinitions, argumentValues, variableValues, +fragmentArgumentValues): + - For each {argumentDefinition} in {argumentDefinitions}: - Let {argumentName} be the name of {argumentDefinition}. - Let {argumentType} be the expected type of {argumentDefinition}. @@ -610,10 +631,15 @@ CoerceArgumentValues(objectType, field, variableValues): {argumentName}. - If {argumentValue} is a {Variable}: - Let {variableName} be the name of {argumentValue}. - - Let {hasValue} be {true} if {variableValues} provides a value for the name - {variableName}. - - Let {value} be the value provided in {variableValues} for the name - {variableName}. + - If {fragmentArgumentValues} provides a value for the name {variableName}: + - Let {hasValue} be {true}. + - Let {value} be the value provided in {fragmentArgumentValues} for the + name {variableName}. + - Otherwise if {variableValues} provides a value for the name + {variableName}: + - Let {hasValue} be {true}. + - Let {value} be the value provided in {variableValues} for the name + {variableName}. - Otherwise, let {value} be {argumentValue}. - If {hasValue} is not {true} and {defaultValue} exists (including {null}): - Add an entry to {coercedValues} named {argumentName} with the value @@ -669,12 +695,12 @@ After resolving the value for a field, it is completed by ensuring it adheres to the expected return type. If the return type is another Object type, then the field execution process continues recursively. -CompleteValue(fieldType, fields, result, variableValues): +CompleteValue(fieldType, fields, result, variableValues, argumentValues): - If the {fieldType} is a Non-Null type: - Let {innerType} be the inner type of {fieldType}. - Let {completedResult} be the result of calling {CompleteValue(innerType, - fields, result, variableValues)}. + fields, result, variableValues, argumentValues)}. - If {completedResult} is {null}, raise a _field error_. - Return {completedResult}. - If {result} is {null} (or another internal value similar to {null} such as @@ -683,8 +709,8 @@ CompleteValue(fieldType, fields, result, variableValues): - If {result} is not a collection of values, raise a _field error_. - Let {innerType} be the inner type of {fieldType}. - Return a list where each list item is the result of calling - {CompleteValue(innerType, fields, resultItem, variableValues)}, where - {resultItem} is each item in {result}. + {CompleteValue(innerType, fields, resultItem, variableValues, + argumentValues)}, where {resultItem} is each item in {result}. - If {fieldType} is a Scalar or Enum type: - Return the result of {CoerceResult(fieldType, result)}. - If {fieldType} is an Object, Interface, or Union type: @@ -694,8 +720,8 @@ CompleteValue(fieldType, fields, result, variableValues): - Let {objectType} be {ResolveAbstractType(fieldType, result)}. - Let {subSelectionSet} be the result of calling {MergeSelectionSets(fields)}. - Return the result of evaluating {ExecuteSelectionSet(subSelectionSet, - objectType, result, variableValues)} _normally_ (allowing for - parallelization). + objectType, result, variableValues, argumentValues)} _normally_ (allowing + for parallelization). **Coercing Results** From 1b580c8fd65b1b9b71a7af8258d5aeed511dbdbb Mon Sep 17 00:00:00 2001 From: Matt Mahoney Date: Wed, 5 May 2021 15:26:13 -0400 Subject: [PATCH 2/7] text changes --- rfcs/FragmentArguments.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/rfcs/FragmentArguments.md b/rfcs/FragmentArguments.md index f48e4d80e..825dac209 100644 --- a/rfcs/FragmentArguments.md +++ b/rfcs/FragmentArguments.md @@ -338,6 +338,8 @@ This validation rule is more strict than necessary: the graphql-js implementation did not require it, given the Overlapping Fields Can Be Merged changes that protect against mis-merged fields. +This validation rule may end up being more strict than required, but it would be easier to relax the rule than make it more strict later. + # Implementation This proposal is implemented completely in From 9b534d98591c3f015b3a294da6bfbdd78bd4578b Mon Sep 17 00:00:00 2001 From: mjmahone Date: Thu, 5 Jan 2023 10:27:26 -0500 Subject: [PATCH 3/7] Address Roman's comments --- spec/Section 2 -- Language.md | 6 +++--- spec/Section 5 -- Validation.md | 37 +++++++++++++++++++-------------- 2 files changed, 24 insertions(+), 19 deletions(-) diff --git a/spec/Section 2 -- Language.md b/spec/Section 2 -- Language.md index 1470f0b42..1b662a8a7 100644 --- a/spec/Section 2 -- Language.md +++ b/spec/Section 2 -- Language.md @@ -1225,7 +1225,7 @@ FragmentArgumentDefinition : Description? Variable : Type DefaultValue? Directives[Const]? Fragments may define locally scoped arguments, which can be used in locations -that accept variables. This allows fragments to be re-used while enabling the +that accept variables. This allows fragments to be reused while enabling the caller to specify the fragment's behavior. For example, the profile picture may need to be a different size depending on @@ -1248,7 +1248,7 @@ fragment dynamicProfilePic($size: Int! = 50) on User { } ``` -In this case, the `user` will have a larger `profilePic` than those found in the +In this case the `user` will have a larger `profilePic` than those found in the list of `friends`. A fragment argument is scoped to the fragment that defines it. Fragment @@ -1277,7 +1277,7 @@ The profilePic for `user` will be determined by the variables set by the operation, while `secondUser` will always have a profilePic of size 10. In this case, the fragment `variableProfilePic` uses the operation defined variable, while `dynamicProfilePic` uses the value passed in via the fragment spread's -argument `size:`. +argument `size`. ## Type References diff --git a/spec/Section 5 -- Validation.md b/spec/Section 5 -- Validation.md index 493025aaf..24e850875 100644 --- a/spec/Section 5 -- Validation.md +++ b/spec/Section 5 -- Validation.md @@ -418,8 +418,14 @@ fragment directFieldSelectionOnUnion on CatOrDog { FieldsInSetCanMerge(set): +- Let {visitedSelections} be the selections in {set} including visiting + fragments and inline fragments an applying any supplied fragment arguments. +- 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. - Let {fieldsForName} be the set of selections with a given response name in - {set} including visiting fragments and inline fragments. + {visitedSelections}. - Given each pair of members {fieldA} and {fieldB} in {fieldsForName}: - {SameResponseShape(fieldA, fieldB)} must be true. - If the parent types of {fieldA} and {fieldB} are equal or if either is not @@ -605,14 +611,14 @@ fragment conflictingFragmentArguments on Dog { } ``` -the response will have two conflicting versions of the `doesKnowCommand` that -cannot merge. +the response will have two conflicting versions of the `doesKnowCommand` +fragment that cannot merge. -One strategy to resolving field conflicts caused by duplicated fragment spreads -is to short-circuit when two fragment spreads with the same name are found to be -merging with different argument values. In this case, validation could short -circuit when `commandFragment(command: SIT)` is merged with -`commandFragment(command: DOWN)`. +If two fragment spreads with the same name supply different argument values, +their fields will not be able to merge. In this case, validation fails because +the fragment spread `...commandFragment(command: SIT)` and +`...commandFragment(command: DOWN)` are part of the visited selections that will +be merged. ### Leaf Field Selections @@ -729,7 +735,7 @@ fragment usesFragmentArg on Dog { } ``` -the following is invalid since `command:` is not defined on +The following is invalid since `command` is not defined on `Dog.doesKnowCommand`. ```graphql counter-example @@ -738,7 +744,7 @@ fragment invalidArgName on Dog { } ``` -and this is also invalid as `$dogCommand` is not defined on fragment +and this is also invalid as the argument `dogCommand` is not defined on fragment `withFragmentArg`. ```graphql counter-example @@ -1892,7 +1898,7 @@ fragment isHouseTrainedWithoutVariableFragment on Dog { ``` Fragment arguments can shadow operation variables: fragments that use an -argument are not using the operation defined variable of the same name. +argument are not using the operation-defined variable of the same name. Likewise, it would be invalid if the variable was shadowed by a fragment argument: @@ -1911,7 +1917,7 @@ fragment shadowedVariableFragment($atOtherHomes: Boolean) on Dog { because {$atOtherHomes} is only referenced in a fragment that defines it as a -locally scoped argument, the operation defined {$atOtherHomes} +locally scoped argument, the operation-defined {$atOtherHomes} variable is never used. All operations in a document must use all of their variables. @@ -1951,11 +1957,10 @@ variable. **Explanatory Text** All arguments defined by a fragment must be used in that same fragment. Because -fragment arguments are scoped to the fragment they're defined on, if the -fragment does not contain a variable with the same name as the argument, then -the argument is superfluous. +fragment arguments are scoped to the fragment they are defined on, if the +fragment does not use the argument, then the argument is superfluous. -For example the following is invalid: +For example, the following is invalid: ```graphql counter-example query queryWithFragmentArgUnused($atOtherHomes: Boolean) { From 03bf02e2e2857627e4f9df18ee2a1351a60676c5 Mon Sep 17 00:00:00 2001 From: mjmahone Date: Thu, 5 Jan 2023 12:42:33 -0500 Subject: [PATCH 4/7] operation-defined --- spec/Section 2 -- Language.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/Section 2 -- Language.md b/spec/Section 2 -- Language.md index 1b662a8a7..270510314 100644 --- a/spec/Section 2 -- Language.md +++ b/spec/Section 2 -- Language.md @@ -1209,7 +1209,7 @@ size `60`: **Variable Use Within Fragments** -Variables can be used within fragments. Operation defined variables have global +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, or by that same fragment as a fragment argument. If a variable is referenced in @@ -1275,7 +1275,7 @@ fragment dynamicProfilePic($size: Int!) on User { The profilePic for `user` will be determined by the variables set by the operation, while `secondUser` will always have a profilePic of size 10. In this -case, the fragment `variableProfilePic` uses the operation defined variable, +case, the fragment `variableProfilePic` uses the operation-defined variable, while `dynamicProfilePic` uses the value passed in via the fragment spread's argument `size`. From 768ca345f411de3f400c42ece9f6d175438c4382 Mon Sep 17 00:00:00 2001 From: mjmahone Date: Thu, 5 Jan 2023 12:55:25 -0500 Subject: [PATCH 5/7] Updated RFC doc to prep for WG --- rfcs/FragmentArguments.md | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/rfcs/FragmentArguments.md b/rfcs/FragmentArguments.md index 825dac209..70f8c6a27 100644 --- a/rfcs/FragmentArguments.md +++ b/rfcs/FragmentArguments.md @@ -289,19 +289,24 @@ fragment Bar($x: Int!) on User { ## Updated Validation: Overlapping Fields Can Be Merged Previously, fragment spreads didn't have to be considered as unique selections -in the overlapping field merging algorithm. However, the algorithm still -de-duplicated common fragment spreads. - -With this change, we can either just treat deduplicated fragment spreads as -being keyed by (name, arguments) rather than just by name, and then apply the -arguments when traversing the child selection set. - -Alternatively, and as implemented in -[graphql-js](https://github.com/graphql/graphql-js/pull/3152), we can both keep -track of the updated fragment keys and also short-circuit whenever we encounter -two fragments with the same name but different arguments. - -## WILL NOT IMPLEMENT Validation Rule: Argument Uniqueness +in the overlapping field merging algorithm. However, in practice the algorithm, +but not the spec, still de-duplicated common fragment spreads. + +With this change, we can just treat deduplicated fragment spreads as being keyed +by (name, arguments) rather than just by name. When visiting child selections, +we need to apply any fragment argument values (basically replace them with +either variable or const values), and then any time we encounter duplicated +fragment spreads with different arguments within merging selections, we consider +that invalid. + +We _could_ just allow field merging rules to apply, but stopping the validation +when same-named fragment spreads with different args are discovered helps +provide much better error messaging and root-causes the issue: the issue isn't +that you reached the same field in the same fragment twice, but rather than you +reached the same fragment spread with different arguments, which will induce +those two usages to be merging the same field with different arguments. + +## WILL NOT IMPLEMENT Validation Rule: Document Argument Uniqueness If the client pre-server compiler rewrites an operation, it's possible to end up with a selection set that violates From aef355c8dea6ba27e10a3b12f212d17055a955e2 Mon Sep 17 00:00:00 2001 From: mjmahone Date: Thu, 5 Jan 2023 13:02:46 -0500 Subject: [PATCH 6/7] Updated RFC doc: explanation on required vs nullable --- rfcs/FragmentArguments.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/rfcs/FragmentArguments.md b/rfcs/FragmentArguments.md index 70f8c6a27..6a29421ec 100644 --- a/rfcs/FragmentArguments.md +++ b/rfcs/FragmentArguments.md @@ -286,6 +286,28 @@ fragment Bar($x: Int!) on User { } ``` +### Potential Alternative: Default Value indicates _required or not_, and `!` indicates _non-null or nullable_. + +If we were writing the language from scratch, I'd advocate for making _all_ +argument definitions without a default value to be required, regardless of their +nullability. If you want to make a nullable argument optional, you do so by +adding a `= null` to its definition. + +In short, if I define: + +``` +fragment Bar($x: Int) { number(x: $x) } +``` + +then `...Bar` would be **invalid**. + +However, that's not how operation variables, field arguments, directive +arguments or input object fields work today, no matter how much I might wish it. +For this RFC, I'm making the meaning of "required" and "nullable" for fragment +spread arguments the same as all other inputs, because doing something +_different_ would be more confusing IMO, even if that difference would lead to +unvalidated fewer foot guns. + ## Updated Validation: Overlapping Fields Can Be Merged Previously, fragment spreads didn't have to be considered as unique selections From c988b54afc72a53f403bafe24c68df0ab6ec8abc Mon Sep 17 00:00:00 2001 From: Matt Mahoney Date: Wed, 11 Jan 2023 16:05:37 -0500 Subject: [PATCH 7/7] Split RFC doc into graphql-wg PR --- rfcs/FragmentArguments.md | 380 -------------------------------------- 1 file changed, 380 deletions(-) delete mode 100644 rfcs/FragmentArguments.md diff --git a/rfcs/FragmentArguments.md b/rfcs/FragmentArguments.md deleted file mode 100644 index 6a29421ec..000000000 --- a/rfcs/FragmentArguments.md +++ /dev/null @@ -1,380 +0,0 @@ -## RFC: Fragment Arguments - -# Problem: Variable Modularity - -GraphQL fragments are designed to allow a client's data requirements to compose. -Two different screens can use the same underlying UI component. If that -component has a corresponding fragment, then each of those screens can include -exactly the data required by having each query spread the child component's -fragment. - -This modularity begins to break down for variables. As an example, let's imagine -a FriendsList component that shows a variable number of friends. We would have a -fragment for that component like so: - -``` -fragment FriendsList on User { - friends(first: $nFriends) { - name - } -} -``` - -In one use, we might want to show some screen-supplied number of friends, and in -another the top 10. For example: - -``` -fragment AnySizedFriendsList on User { - name - ...FriendsList -} - -fragment TopFriendsUserProfile on User { - name - profile_picture { uri } - ...FriendsList -} -``` - -Even though every usage of `TopFriendsUserProfile` should be setting `$nFriends` -to `10`, the only way to enforce that is by manually walking all callers of -`TopFriendsUserProfile`, recursively, until you arrive at the operation -definition and verify the variable is defined like `$nFriends: Int = 10`. - -This causes a few major usability problems: - -- If I ever want to change the number of items `TopFriendsUserProfile` includes, - I now need to update every _operation_ that includes it. This could be dozens - or hundreds of individual locations. -- Even if the component for `TopFriendsUserProfile` is only able to display 10 - friends, in most clients at runtime the user can override the default value, - enabling a mismatch between the data required and the data asked for. - -# Existing Solution: Relay's `@arguments`/`@argumentDefinitions` - -Relay has a solution for this problem by using a custom, non-spec-compliant pair -of directives, -[`@arguments`/`@argumentDefinitions`](https://relay.dev/docs/api-reference/graphql-and-directives/#arguments). - -These directives live only in user-facing GraphQL definitions, and are compiled -away prior to making a server request. - -Following the above example, if we were using Relay we'd be able to write: - -``` -fragment FriendsList on User @argumentDefinitions(nFriends: {type: "Int!"}) { - friends(first: $nFriends) { - name - } -} - -fragment AnySizedFriendsList on User { - name - ...FriendsList @arguments(nFriends: $operationProvidedFriendCount) -} - -fragment TopFriendsUserProfile on User { - name - profile_picture { uri } - ...FriendsList @arguments(nFriends: 10) -} -``` - -Before sending a query to the server, Relay compiles away these directives so -the server, when running an operation using `TopFriendsUserProfile`, sees: - -``` -fragment FriendsList on User { - friends(first: 10) { - name - } -} - -fragment TopFriendsUserProfile on User { - name - profile_picture { uri } - ...FriendsList -} -``` - -The exact mechanics of how Relay rewrites the user-written operation based on -`@arguments` supplied is not the focus of this RFC. - -However, even to enable this client-compile-time operation transformation, Relay -had to introduce _non-compliant directives_: each argument to `@arguments` -changes based on the fragment the directive is applied to. While syntactically -valid, this fails the -[Argument Names validation](https://spec.graphql.org/draft/#sec-Argument-Names). - -Additionally, the `@argumentDefinitions` directive gets very verbose and unsafe, -using strings to represent variable type declarations. - -Relay has supported `@arguments` in its current form since -[v2.0](https://github.com/facebook/relay/releases/tag/v2.0.0), released in -January 2019. There's now a large body of evidence that allowing fragments to -define arguments that can be passed into fragment spreads is a significant -usability improvement, and valuable to the wider GraphQL community. However, if -we are to introduce this notion more broadly, we should make sure the ergonomics -of it conform to users' expectations. - -# Proposal: Introduce Fragment Argument Definitions, which allow using arguments on Fragment Spreads - -Relay's `@arguments`/`@argumentDefinitions` concepts provide value, and can be -applied against GraphQL written for existing GraphQL servers so long as there is -a pre-server compiler which transforms the concept away. - -## New Fragment Argument Definition syntax - -For the `@argumentDefinitions` concept, we can allow fragments to share the same -syntax as operation level definitions. Going back to the previous example, this -would look like: - -``` -fragment FriendsList($nFriends: Int!) on User { - friends(first: $nFriends) { - name - } -} -``` - -The syntax re-uses the concepts from Variable Definitions, so that when you -_define_ and _use_ the argument, it preserves the same appearance (`$` + name). - -## New Fragment Spread Argument syntax - -For the `@arguments` concept, we can allow fragment spreads to share the same -syntax as field and directive arguments. - -``` -fragment AnySizedFriendsList on User { - name - ...FriendsList(nFriends: $operationProvidedFriendCount) -} - -fragment TopFriendsUserProfile on User { - name - profile_picture { uri } - ...FriendsList(nFriends: 10) -} -``` - -This may feel a little weird: for fields, arguments are defined as -`argName: Type` and then used like `...Foo(argName: $variable)`. The -alternatives here are: - -- Define `argName: Type` for fragment arguments - - This has the disadvantage of seeing both the argument definition and the - argument usage in the same fragment with different styles. -- Call `...Foo($argName: $variable)` - - This feels incredibly confusing: `$` typically means "replace this token - with a value", and that's not what's happening. - -Notably, this proposed syntax, of using `$name` at the definition and usage -site, and `name:` when calling the Fragment/Function, is the convention that PHP -uses for -[Named Arguments](https://www.php.net/manual/en/functions.arguments.php#functions.named-arguments). -Given GraphQL was designed with many PHP-isms, it seems like we should re-use -the conventions chosen there when there's no clear reason not to. - -## Scope: Local - -Fragment Arguments should always have local scope. This gets us closer to the -idea that while operations are "global", fragments behave more like well-scoped -functions. - -This has a bunch of beneficial side effects: - -- For validation, we don't need ot check for fragment argument definition - clashes -- For composability, I can use the same argument on many fragments and not worry - about unrelated fragments. -- For ease-of-understanding, you don't need to keep track of how all child - fragments use a fragment argument to understand how changing something like - the default value will modify the results. -- Makes it easy to update Variables In Allowed Positions, as we don't need to - hunt the definition of a variable across many potential parent fragments. - -The other scoping options are: - -- Global, i.e. a fragment argument is just syntactic sugar for an operation - variable. - - This is what we implemented at Meta, and it was terrible for all the reasons - you can think of. -- Recursively local, i.e. the variable takes on any parent fragment argument - definition - - This preserves the concept of the value being some sort of recursively - scoped variable. - - However, as explained above, keeping track of what's happening, and - preventing fragment conflicts, becomes really difficult. - -We're choosing to explicitly _allow_ overriding operation variables, as the -local scope means you can clearly see whether a variable is scoped to the -fragment or operation. - -## New Validation Rule: Fragment Argument Definitions Used in Fragment - -With local scope, this rule is very simple. - -``` -fragment Foo($x: Int) on User { - name -} -``` - -would be invalid. - -Additionally, - -``` -fragment Foo($x: Int!) on User { - ...Bar -} - -fragment Bar { - number(x: $x) -} -``` - -would also be invalid: even though `$x` is used underneath Foo, it is used -outside of Foo's explicit definition. In this context, `$x` in Bar is actually -an operation variable. - -However, this would be valid: - -``` -fragment Foo($x: Int!) on User { - ...Bar(x: $x) -} - -fragment Bar($x: Int) { - number(x: $x) -} -``` - -### Consideration: how strict should this rule be? - -As an initial RFC, I'd advocate for encouraging the _strictest_ version of this -rule possible: any argument defined on a fragment must be explicitly used by -that same fragment. It would be easy to relax the rule later, but very difficult -to do the reverse. - -It's clearly more composable if, when changing a child fragment, you don't need -to worry about modifying argument definitions on parent fragments callsites. -However, we could in the future allow annotating argument definitions with -`@deprecated`. - -## Updated Validation Rule: Required Arguments are Provided - -We update -[Required Arguments](https://spec.graphql.org/draft/#sec-Required-Arguments) to -include fragment spreads. This makes the validation's first two bullets: - -- For each Field, **Fragment Spread** or Directive in the document. -- Let _arguments_ be the set of argument definitions of that Field, **Fragment** - or Directive. - -With this rule, the below example is invalid, even if the argument -`User.number(x:)` is nullable in the schema. - -``` -fragment Foo on User { - ...Bar -} - -fragment Bar($x: Int!) on User { - number(x: $x) -} -``` - -### Potential Alternative: Default Value indicates _required or not_, and `!` indicates _non-null or nullable_. - -If we were writing the language from scratch, I'd advocate for making _all_ -argument definitions without a default value to be required, regardless of their -nullability. If you want to make a nullable argument optional, you do so by -adding a `= null` to its definition. - -In short, if I define: - -``` -fragment Bar($x: Int) { number(x: $x) } -``` - -then `...Bar` would be **invalid**. - -However, that's not how operation variables, field arguments, directive -arguments or input object fields work today, no matter how much I might wish it. -For this RFC, I'm making the meaning of "required" and "nullable" for fragment -spread arguments the same as all other inputs, because doing something -_different_ would be more confusing IMO, even if that difference would lead to -unvalidated fewer foot guns. - -## Updated Validation: Overlapping Fields Can Be Merged - -Previously, fragment spreads didn't have to be considered as unique selections -in the overlapping field merging algorithm. However, in practice the algorithm, -but not the spec, still de-duplicated common fragment spreads. - -With this change, we can just treat deduplicated fragment spreads as being keyed -by (name, arguments) rather than just by name. When visiting child selections, -we need to apply any fragment argument values (basically replace them with -either variable or const values), and then any time we encounter duplicated -fragment spreads with different arguments within merging selections, we consider -that invalid. - -We _could_ just allow field merging rules to apply, but stopping the validation -when same-named fragment spreads with different args are discovered helps -provide much better error messaging and root-causes the issue: the issue isn't -that you reached the same field in the same fragment twice, but rather than you -reached the same fragment spread with different arguments, which will induce -those two usages to be merging the same field with different arguments. - -## WILL NOT IMPLEMENT Validation Rule: Document Argument Uniqueness - -If the client pre-server compiler rewrites an operation, it's possible to end up -with a selection set that violates -[Field Selection Merging](https://spec.graphql.org/draft/#sec-Field-Selection-Merging) -validation. Additionally, we have no mechanism on servers today to handle the -same fragment having different variable values depending on that fragment's -location in an operation. - -Therefore, any Fragment Spread for the same Fragment in an Operation could be -required to have non-conflicting argument values passed in. - -As an example, this is invalid: - -``` -query { - user { - best_friend { - ...UserProfile(imageSize: 100) - } - ...UserProfile(imageSize: 200) - } -} -``` - -Note: today Relay's compiler handles this ambiguity. In an extreme -simplification, this is done by producing two unique versions of `UserProfile`, -where in `UserProfile_0` `$imageSize` is replaced with `100`, and in -`UserProfile_1` `$imageSize` is replaced with `200`. However, there exist client -implementations that are unable to have multiple applications of the same -fragment within a single operation (the clients I work on cannot use Relay's -trick). - -This validation rule is more strict than necessary: the graphql-js -implementation did not require it, given the Overlapping Fields Can Be Merged -changes that protect against mis-merged fields. - -This validation rule may end up being more strict than required, but it would be easier to relax the rule than make it more strict later. - -# Implementation - -This proposal is implemented completely in -[graphql-js](https://github.com/graphql/graphql-js/pull/3152) - -## Initial Implementation - -In the initial implementation, I tried to change as little as possible. This -means I only added a single new validation rule. Additionally, there may be some -weirdness around internal grammar and AST node naming/usage, but the actual -behavior should be feature complete.