Skip to content

Commit bc87fa8

Browse files
mjmahonemjmahone
committed
Fragment Arguments added to spec
1 parent ab865f9 commit bc87fa8

7 files changed

+624
-50
lines changed

‎rfcs/FragmentArguments.md

Lines changed: 351 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,351 @@
1+
## RFC: Fragment Arguments
2+
3+
# Problem: Variable Modularity
4+
5+
GraphQL fragments are designed to allow a client's data requirements to compose.
6+
Two different screens can use the same underlying UI component. If that
7+
component has a corresponding fragment, then each of those screens can include
8+
exactly the data required by having each query spread the child component's
9+
fragment.
10+
11+
This modularity begins to break down for variables. As an example, let's imagine
12+
a FriendsList component that shows a variable number of friends. We would have a
13+
fragment for that component like so:
14+
15+
```
16+
fragment FriendsList on User {
17+
friends(first: $nFriends) {
18+
name
19+
}
20+
}
21+
```
22+
23+
In one use, we might want to show some screen-supplied number of friends, and in
24+
another the top 10. For example:
25+
26+
```
27+
fragment AnySizedFriendsList on User {
28+
name
29+
...FriendsList
30+
}
31+
32+
fragment TopFriendsUserProfile on User {
33+
name
34+
profile_picture { uri }
35+
...FriendsList
36+
}
37+
```
38+
39+
Even though every usage of `TopFriendsUserProfile` should be setting `$nFriends`
40+
to `10`, the only way to enforce that is by manually walking all callers of
41+
`TopFriendsUserProfile`, recursively, until you arrive at the operation
42+
definition and verify the variable is defined like `$nFriends: Int = 10`.
43+
44+
This causes a few major usability problems:
45+
46+
- If I ever want to change the number of items `TopFriendsUserProfile` includes,
47+
I now need to update every _operation_ that includes it. This could be dozens
48+
or hundreds of individual locations.
49+
- Even if the component for `TopFriendsUserProfile` is only able to display 10
50+
friends, in most clients at runtime the user can override the default value,
51+
enabling a mismatch between the data required and the data asked for.
52+
53+
# Existing Solution: Relay's `@arguments`/`@argumentDefinitions`
54+
55+
Relay has a solution for this problem by using a custom, non-spec-compliant pair
56+
of directives,
57+
[`@arguments`/`@argumentDefinitions`](https://relay.dev/docs/api-reference/graphql-and-directives/#arguments).
58+
59+
These directives live only in user-facing GraphQL definitions, and are compiled
60+
away prior to making a server request.
61+
62+
Following the above example, if we were using Relay we'd be able to write:
63+
64+
```
65+
fragment FriendsList on User @argumentDefinitions(nFriends: {type: "Int!"}) {
66+
friends(first: $nFriends) {
67+
name
68+
}
69+
}
70+
71+
fragment AnySizedFriendsList on User {
72+
name
73+
...FriendsList @arguments(nFriends: $operationProvidedFriendCount)
74+
}
75+
76+
fragment TopFriendsUserProfile on User {
77+
name
78+
profile_picture { uri }
79+
...FriendsList @arguments(nFriends: 10)
80+
}
81+
```
82+
83+
Before sending a query to the server, Relay compiles away these directives so
84+
the server, when running an operation using `TopFriendsUserProfile`, sees:
85+
86+
```
87+
fragment FriendsList on User {
88+
friends(first: 10) {
89+
name
90+
}
91+
}
92+
93+
fragment TopFriendsUserProfile on User {
94+
name
95+
profile_picture { uri }
96+
...FriendsList
97+
}
98+
```
99+
100+
The exact mechanics of how Relay rewrites the user-written operation based on
101+
`@arguments` supplied is not the focus of this RFC.
102+
103+
However, even to enable this client-compile-time operation transformation, Relay
104+
had to introduce _non-compliant directives_: each argument to `@arguments`
105+
changes based on the fragment the directive is applied to. While syntactically
106+
valid, this fails the
107+
[Argument Names validation](https://spec.graphql.org/draft/#sec-Argument-Names).
108+
109+
Additionally, the `@argumentDefinitions` directive gets very verbose and unsafe,
110+
using strings to represent variable type declarations.
111+
112+
Relay has supported `@arguments` in its current form since
113+
[v2.0](https://github.com/facebook/relay/releases/tag/v2.0.0), released in
114+
January 2019. There's now a large body of evidence that allowing fragments to
115+
define arguments that can be passed into fragment spreads is a significant
116+
usability improvement, and valuable to the wider GraphQL community. However, if
117+
we are to introduce this notion more broadly, we should make sure the ergonomics
118+
of it conform to users' expectations.
119+
120+
# Proposal: Introduce Fragment Argument Definitions, which allow using arguments on Fragment Spreads
121+
122+
Relay's `@arguments`/`@argumentDefinitions` concepts provide value, and can be
123+
applied against GraphQL written for existing GraphQL servers so long as there is
124+
a pre-server compiler which transforms the concept away.
125+
126+
## New Fragment Argument Definition syntax
127+
128+
For the `@argumentDefinitions` concept, we can allow fragments to share the same
129+
syntax as operation level definitions. Going back to the previous example, this
130+
would look like:
131+
132+
```
133+
fragment FriendsList($nFriends: Int!) on User {
134+
friends(first: $nFriends) {
135+
name
136+
}
137+
}
138+
```
139+
140+
The syntax re-uses the concepts from Variable Definitions, so that when you
141+
_define_ and _use_ the argument, it preserves the same appearance (`$` + name).
142+
143+
## New Fragment Spread Argument syntax
144+
145+
For the `@arguments` concept, we can allow fragment spreads to share the same
146+
syntax as field and directive arguments.
147+
148+
```
149+
fragment AnySizedFriendsList on User {
150+
name
151+
...FriendsList(nFriends: $operationProvidedFriendCount)
152+
}
153+
154+
fragment TopFriendsUserProfile on User {
155+
name
156+
profile_picture { uri }
157+
...FriendsList(nFriends: 10)
158+
}
159+
```
160+
161+
This may feel a little weird: for fields, arguments are defined as
162+
`argName: Type` and then used like `...Foo(argName: $variable)`. The
163+
alternatives here are:
164+
165+
- Define `argName: Type` for fragment arguments
166+
- This has the disadvantage of seeing both the argument definition and the
167+
argument usage in the same fragment with different styles.
168+
- Call `...Foo($argName: $variable)`
169+
- This feels incredibly confusing: `$` typically means "replace this token
170+
with a value", and that's not what's happening.
171+
172+
Notably, this proposed syntax, of using `$name` at the definition and usage
173+
site, and `name:` when calling the Fragment/Function, is the convention that PHP
174+
uses for
175+
[Named Arguments](https://www.php.net/manual/en/functions.arguments.php#functions.named-arguments).
176+
Given GraphQL was designed with many PHP-isms, it seems like we should re-use
177+
the conventions chosen there when there's no clear reason not to.
178+
179+
## Scope: Local
180+
181+
Fragment Arguments should always have local scope. This gets us closer to the
182+
idea that while operations are "global", fragments behave more like well-scoped
183+
functions.
184+
185+
This has a bunch of beneficial side effects:
186+
187+
- For validation, we don't need ot check for fragment argument definition
188+
clashes
189+
- For composability, I can use the same argument on many fragments and not worry
190+
about unrelated fragments.
191+
- For ease-of-understanding, you don't need to keep track of how all child
192+
fragments use a fragment argument to understand how changing something like
193+
the default value will modify the results.
194+
- Makes it easy to update Variables In Allowed Positions, as we don't need to
195+
hunt the definition of a variable across many potential parent fragments.
196+
197+
The other scoping options are:
198+
199+
- Global, i.e. a fragment argument is just syntactic sugar for an operation
200+
variable.
201+
- This is what we implemented at Meta, and it was terrible for all the reasons
202+
you can think of.
203+
- Recursively local, i.e. the variable takes on any parent fragment argument
204+
definition
205+
- This preserves the concept of the value being some sort of recursively
206+
scoped variable.
207+
- However, as explained above, keeping track of what's happening, and
208+
preventing fragment conflicts, becomes really difficult.
209+
210+
We're choosing to explicitly _allow_ overriding operation variables, as the
211+
local scope means you can clearly see whether a variable is scoped to the
212+
fragment or operation.
213+
214+
## New Validation Rule: Fragment Argument Definitions Used in Fragment
215+
216+
With local scope, this rule is very simple.
217+
218+
```
219+
fragment Foo($x: Int) on User {
220+
name
221+
}
222+
```
223+
224+
would be invalid.
225+
226+
Additionally,
227+
228+
```
229+
fragment Foo($x: Int!) on User {
230+
...Bar
231+
}
232+
233+
fragment Bar {
234+
number(x: $x)
235+
}
236+
```
237+
238+
would also be invalid: even though `$x` is used underneath Foo, it is used
239+
outside of Foo's explicit definition. In this context, `$x` in Bar is actually
240+
an operation variable.
241+
242+
However, this would be valid:
243+
244+
```
245+
fragment Foo($x: Int!) on User {
246+
...Bar(x: $x)
247+
}
248+
249+
fragment Bar($x: Int) {
250+
number(x: $x)
251+
}
252+
```
253+
254+
### Consideration: how strict should this rule be?
255+
256+
As an initial RFC, I'd advocate for encouraging the _strictest_ version of this
257+
rule possible: any argument defined on a fragment must be explicitly used by
258+
that same fragment. It would be easy to relax the rule later, but very difficult
259+
to do the reverse.
260+
261+
It's clearly more composable if, when changing a child fragment, you don't need
262+
to worry about modifying argument definitions on parent fragments callsites.
263+
However, we could in the future allow annotating argument definitions with
264+
`@deprecated`.
265+
266+
## Updated Validation Rule: Required Arguments are Provided
267+
268+
We update
269+
[Required Arguments](https://spec.graphql.org/draft/#sec-Required-Arguments) to
270+
include fragment spreads. This makes the validation's first two bullets:
271+
272+
- For each Field, **Fragment Spread** or Directive in the document.
273+
- Let _arguments_ be the set of argument definitions of that Field, **Fragment**
274+
or Directive.
275+
276+
With this rule, the below example is invalid, even if the argument
277+
`User.number(x:)` is nullable in the schema.
278+
279+
```
280+
fragment Foo on User {
281+
...Bar
282+
}
283+
284+
fragment Bar($x: Int!) on User {
285+
number(x: $x)
286+
}
287+
```
288+
289+
## Updated Validation: Overlapping Fields Can Be Merged
290+
291+
Previously, fragment spreads didn't have to be considered as unique selections
292+
in the overlapping field merging algorithm. However, the algorithm still
293+
de-duplicated common fragment spreads.
294+
295+
With this change, we can either just treat deduplicated fragment spreads as
296+
being keyed by (name, arguments) rather than just by name, and then apply the
297+
arguments when traversing the child selection set.
298+
299+
Alternatively, and as implemented in
300+
[graphql-js](https://github.com/graphql/graphql-js/pull/3152), we can both keep
301+
track of the updated fragment keys and also short-circuit whenever we encounter
302+
two fragments with the same name but different arguments.
303+
304+
## WILL NOT IMPLEMENT Validation Rule: Argument Uniqueness
305+
306+
If the client pre-server compiler rewrites an operation, it's possible to end up
307+
with a selection set that violates
308+
[Field Selection Merging](https://spec.graphql.org/draft/#sec-Field-Selection-Merging)
309+
validation. Additionally, we have no mechanism on servers today to handle the
310+
same fragment having different variable values depending on that fragment's
311+
location in an operation.
312+
313+
Therefore, any Fragment Spread for the same Fragment in an Operation could be
314+
required to have non-conflicting argument values passed in.
315+
316+
As an example, this is invalid:
317+
318+
```
319+
query {
320+
user {
321+
best_friend {
322+
...UserProfile(imageSize: 100)
323+
}
324+
...UserProfile(imageSize: 200)
325+
}
326+
}
327+
```
328+
329+
Note: today Relay's compiler handles this ambiguity. In an extreme
330+
simplification, this is done by producing two unique versions of `UserProfile`,
331+
where in `UserProfile_0` `$imageSize` is replaced with `100`, and in
332+
`UserProfile_1` `$imageSize` is replaced with `200`. However, there exist client
333+
implementations that are unable to have multiple applications of the same
334+
fragment within a single operation (the clients I work on cannot use Relay's
335+
trick).
336+
337+
This validation rule is more strict than necessary: the graphql-js
338+
implementation did not require it, given the Overlapping Fields Can Be Merged
339+
changes that protect against mis-merged fields.
340+
341+
# Implementation
342+
343+
This proposal is implemented completely in
344+
[graphql-js](https://github.com/graphql/graphql-js/pull/3152)
345+
346+
## Initial Implementation
347+
348+
In the initial implementation, I tried to change as little as possible. This
349+
means I only added a single new validation rule. Additionally, there may be some
350+
weirdness around internal grammar and AST node naming/usage, but the actual
351+
behavior should be feature complete.

‎spec/Appendix B -- Grammar Summary.md

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -168,15 +168,20 @@ Arguments[Const] : ( Argument[?Const]+ )
168168

169169
Argument[Const] : Name : Value[?Const]
170170

171-
FragmentSpread : ... FragmentName Directives?
171+
FragmentSpread : ... FragmentName Arguments? Directives?
172172

173173
InlineFragment : ... TypeCondition? Directives? SelectionSet
174174

175-
FragmentDefinition : fragment FragmentName TypeCondition Directives?
176-
SelectionSet
175+
FragmentDefinition : fragment FragmentName FragmentArgumentsDefinition?
176+
TypeCondition Directives? SelectionSet
177177

178178
FragmentName : Name but not `on`
179179

180+
FragmentArgumentsDefinition : ( FragmentArgumentDefinition+ )
181+
182+
FragmentArgumentDefinition : Description? Variable : Type DefaultValue?
183+
Directives[Const]?
184+
180185
TypeCondition : on NamedType
181186

182187
Value[Const] :
@@ -396,6 +401,7 @@ ExecutableDirectiveLocation : one of
396401
- `FRAGMENT_SPREAD`
397402
- `INLINE_FRAGMENT`
398403
- `VARIABLE_DEFINITION`
404+
- `FRAGMENT_ARGUMENT_DEFINITION`
399405

400406
TypeSystemDirectiveLocation : one of
401407

0 commit comments

Comments
 (0)