Skip to content

Commit 4f277c1

Browse files
mjmahonemjmahone
committed
RFC: Fragment Arguments
1 parent 8a0a4b0 commit 4f277c1

File tree

1 file changed

+380
-0
lines changed

1 file changed

+380
-0
lines changed

‎rfcs/FragmentArguments.md

Lines changed: 380 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,380 @@
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+
### Potential Alternative: Default Value indicates _required or not_, and `!` indicates _non-null or nullable_.
290+
291+
If we were writing the language from scratch, I'd advocate for making _all_
292+
argument definitions without a default value to be required, regardless of their
293+
nullability. If you want to make a nullable argument optional, you do so by
294+
adding a `= null` to its definition.
295+
296+
In short, if I define:
297+
298+
```
299+
fragment Bar($x: Int) { number(x: $x) }
300+
```
301+
302+
then `...Bar` would be **invalid**.
303+
304+
However, that's not how operation variables, field arguments, directive
305+
arguments or input object fields work today, no matter how much I might wish it.
306+
For this RFC, I'm making the meaning of "required" and "nullable" for fragment
307+
spread arguments the same as all other inputs, because doing something
308+
_different_ would be more confusing IMO, even if that difference would lead to
309+
unvalidated fewer foot guns.
310+
311+
## Updated Validation: Overlapping Fields Can Be Merged
312+
313+
Previously, fragment spreads didn't have to be considered as unique selections
314+
in the overlapping field merging algorithm. However, in practice the algorithm,
315+
but not the spec, still de-duplicated common fragment spreads.
316+
317+
With this change, we can just treat deduplicated fragment spreads as being keyed
318+
by (name, arguments) rather than just by name. When visiting child selections,
319+
we need to apply any fragment argument values (basically replace them with
320+
either variable or const values), and then any time we encounter duplicated
321+
fragment spreads with different arguments within merging selections, we consider
322+
that invalid.
323+
324+
We _could_ just allow field merging rules to apply, but stopping the validation
325+
when same-named fragment spreads with different args are discovered helps
326+
provide much better error messaging and root-causes the issue: the issue isn't
327+
that you reached the same field in the same fragment twice, but rather than you
328+
reached the same fragment spread with different arguments, which will induce
329+
those two usages to be merging the same field with different arguments.
330+
331+
## WILL NOT IMPLEMENT Validation Rule: Document Argument Uniqueness
332+
333+
If the client pre-server compiler rewrites an operation, it's possible to end up
334+
with a selection set that violates
335+
[Field Selection Merging](https://spec.graphql.org/draft/#sec-Field-Selection-Merging)
336+
validation. Additionally, we have no mechanism on servers today to handle the
337+
same fragment having different variable values depending on that fragment's
338+
location in an operation.
339+
340+
Therefore, any Fragment Spread for the same Fragment in an Operation could be
341+
required to have non-conflicting argument values passed in.
342+
343+
As an example, this is invalid:
344+
345+
```
346+
query {
347+
user {
348+
best_friend {
349+
...UserProfile(imageSize: 100)
350+
}
351+
...UserProfile(imageSize: 200)
352+
}
353+
}
354+
```
355+
356+
Note: today Relay's compiler handles this ambiguity. In an extreme
357+
simplification, this is done by producing two unique versions of `UserProfile`,
358+
where in `UserProfile_0` `$imageSize` is replaced with `100`, and in
359+
`UserProfile_1` `$imageSize` is replaced with `200`. However, there exist client
360+
implementations that are unable to have multiple applications of the same
361+
fragment within a single operation (the clients I work on cannot use Relay's
362+
trick).
363+
364+
This validation rule is more strict than necessary: the graphql-js
365+
implementation did not require it, given the Overlapping Fields Can Be Merged
366+
changes that protect against mis-merged fields.
367+
368+
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.
369+
370+
# Implementation
371+
372+
This proposal is implemented completely in
373+
[graphql-js](https://github.com/graphql/graphql-js/pull/3152)
374+
375+
## Initial Implementation
376+
377+
In the initial implementation, I tried to change as little as possible. This
378+
means I only added a single new validation rule. Additionally, there may be some
379+
weirdness around internal grammar and AST node naming/usage, but the actual
380+
behavior should be feature complete.

0 commit comments

Comments
 (0)