Skip to content

Latest commit

 

History

History
380 lines (294 loc) · 13.1 KB

FragmentArguments.md

File metadata and controls

380 lines (294 loc) · 13.1 KB

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.

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.

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, 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. 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 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 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

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.