RFC: Fragment Arguments for Client GraphQL

[mjmahone]

This is a proposal to bring Relay-style Fragment Arguments into the Spec as an optional, client-only language feature.

Overview

We would allow clients to write GraphQL like:

query Q1 {
  ...Number(x: 100)
}

query Q2($i: Int = 3) {
  ...Number(x: $i)
}

fragment Number($x: Int!) on Query {
  number(value: $x)
}

which would, prior to sending to the server, be transformed into two unique queries that current spec-compliant servers would understand, such that it behaved like the queries were written like:

query Q1 {
  ...Number
}

fragment Number on Query {
  number(value: 100)
}

and

query Q2($i: Int = 3) {
  ...Number
}

fragment Number on Query {
  number(value: $i)
}

The exact mechanics of how the query gets rewritten could be different per client, but the behavior should be identical. This RFC seeks to describe the new syntax, as well as adding additional validation for clients that choose to support fragment arguments.

Background

Relay has seen a lot of usage of their non-spec-compliant @arguments and @argumentDefinitions directives, but they're both cumbersome to use and fail basic Spec validation (directives with arguments used but never defined). They accomplish this by pre-compiling any GraphQL definitions using @arguments and @argumentDefinitions to transform the document such that, by the time it's sent to the server, it is spec compliant. However, Relay's user-facing directives both do not conform to the spec and also provide a pretty cumbersome, somewhat unintuitive user-facing design.

This proposal is to allow clients that pre-compile their GraphQL prior to sending it to the server (such as Relay) to have a built-in syntax for passing argument values into fragment spreads for fragments with well-defined argument definitions.

Champion: @mjmahone

[andimarek]

@mjmahone some general questions:

• This would split GraphQL syntax between client and server. Is that correct?
• Why not make it a full feature which are implemented by the server?
• What is the practical reason to bring this into the spec: change of client side validation rule?

thanks

[mjmahone]

This would split GraphQL syntax between client and server. Is that correct?

Yes, but that's already true for clients like Relay today. This spec proposal would have the actual spec basically accepting that current reality.

Why not make it a full feature which are implemented by the server?

I would like to, but there's a lot higher of a bar to this. Especially in terms of backwards compatibility: this spec RFC provides a way for a client hitting any current-spec-compliant server implementation to use these new idioms, without waiting for a server upgrade.

Basically, I anticipate more changes in this space, including changes to server execution behavior, but I'd be loathe to block a client from using the new paradigm until after we have the server-side figured out.

Importantly: this paradigm is widely used within FB inside Relay, but the ergonomics of @arguments/@argumentDefinitions is a major complaint. We currently face the chicken and egg problem, though: Prettier and GraphiQL are incredibly important tools for us, and we can't use the better syntax for Relay internally until Prettier and GraphiQL support it, but those repos don't have a path right now to supporting non-graphql-js-supported syntax, and graphql-js explicitly only supports spec syntax (or spec-RFCs under feature flags).

What is the practical reason to bring this into the spec: change of client side validation rule?

In short, tooling. GraphiQL and Prettier's GraphQL plugin both rely on the spec's interpretation of "what GraphQL is". In some sense, that eliminates our ability to iterate on "future GraphQL": if we need both the server and client to agree on any new "spec features", even when the client could "transpile" to previous-spec-GraphQL (as people call the process of JS-new to JS-old compilation), we end up in a chicken-and-egg situation for introducing new paradigms. New paradigms, without tooling support, can't get adopted.

An alternative to introducing this in the spec would be to introduce it purely in the tooling repos: this is certainly possible, but if graphql-js is meant to exactly match the spec + spec RFCs (with feature flags), that precludes tooling from relying on graphql-js: in this world, either people can't use GraphiQL and Prettier with the new syntax, or GraphiQL needs a custom AST that it locally compiles to graphql-js's AST (and will need to constantly update the custom AST with any new graphql-js features), and Prettier will have to rely completely on a custom AST that needs to stay in sync with graphql-js.

It seems better IMO to put an "optional extension" into the spec "if we allowed fragment arguments, this is the syntax we'd use. For clients using the new syntax to stay compatible with the current server spec, here's the limitations we need to add via extra validation rules".

[IvanGoncharov]

@mjmahone In the long run, we need to switch graphql-js to have an execution plan stage before executing the request (since it what the majority of other production-ready implementations already did). And having an execution plan with all arguments coerced and prepared for every field means performance and code complexity of this feature would fully handle during the creation of the execution plan.

My proposal is to merge it as RFC and implement parsing and validation in graphql-js but create a separate validation rule (enabled by default) that forbids argument definitions on fragments. That way GraphiQL and another tooling can just disable this single rule until we implement this feature on the backend.

[mjmahone]

I actually think the executor implementation is not that difficult: https://github.com/graphql/graphql-js/pull/3152. I'll iterate on the spec changes required once we have some form of consensus that this experimental implementation is OK to iterate with.

[ivome]

I think it would be awesome if we could add this without the rule "Fragment Argument Uniqueness". I don't see a logical reason from a spec point of view why this rule needs to exist. It might be more complex to implement on the server or client, but not having this rule in place would have huge benefits IMO:

With the rule in place, a user of a fragment has to have knowledge of the entire query tree to know if the fragment can be used in a certain context. Without the rule, all dependencies can be explicitly defined throughout the entire query tree, and a component that uses a fragment can be used any number of times with any variables in any context.

As an idea, we could add a rule that all the variables that are used within a fragment have to be defined on the fragment definition.

Valid example

query Query($large: Int!, $small: Int!) {
  user {
    ...ProfileImage(size: $large)
     friends {
       ...ProfileImage(size: $small)
     }
  }
}

fragment ProfileImage($size: Int!) on User {
  imageUrl(size: $size)
}

Invalid example This would be invalid because the used variable is not defined in the fragment definition:

fragment ProfileImage on User {
  imageUrl(size: $size)
}

Benefits of this approach:

• Fragments that use variables become modular and composable. We eliminate the dependency between fragment and query definitions which has to match right now. This would make it easier to scale complex queries.
• We eliminate errors that arise when a user accidentally creates a fragment variable with a name that is already used in another part of a query with a different type for example.
• No knowledge required anymore by fragment users and implementors since all dependencies are explicitly defined right where they are used.
• We can validate individual fragments without having to know the query in which they will be used.
• Allows us to implement better autocomplete features in developer tools.

To ensure backward compatibility, we can detect if there are variables passed to fragments in the query definition. If there are fragment variables passed, the query is executed with the new strict logic, otherwise, the current validation rules apply.

[mjmahone]

@ivome: I agree that making fragments truly modular and composable is desirable. There's a lot that can be done, but what you've proposed won't by itself solve the issue. If the goal is to make fragments fully modular, I think you'd do well to work through as many counterexamples as you can for your proposal.

How would you prevent this?

query Query {
  user {
    ...SmallUser
    ...BigUser
  }
}

fragment SmallUser on User {
  ...ProfileImage(size: 10)
}

fragment BigUser on User {
  ...ProfileImage(size: 100)
}

fragment ProfileImage($size: Int!) on User {
  imageUrl(size: $size)
}

In short: GraphQL is already not modular with respect to fragments. This proposal is not trying to solve all modularity problems, but to make it possible to solve some of them (primarily, allowing fragments to be used without an operation needing to know about that fragment's variables directly).

There are probably a series of modifications we could make to the language to get GraphQL to be truly modular, but that's outside this RFC's scope.

[ivome]

How would you prevent this?

This particular case would be prevented by the rules defined for field selection merging: https://spec.graphql.org/June2018/#sec-Field-Selection-Merging

In short: GraphQL is already not modular with respect to fragments. This proposal is not trying to solve all modularity problems, but to make it possible to solve some of them (primarily, allowing fragments to be used without an operation needing to know about that fragment's variables directly).

There are probably a series of modifications we could make to the language to get GraphQL to be truly modular, but that's outside this RFC's scope.

I agree, GraphQL is not 100% modular and it would require major changes to make it that way. What I'm proposing here is related to a similar point that @leebyron raised in his latest comment on your PR graphql/graphql-js#3152:

"Unique variable names" - should a fragment variable be able to name override another variable?

In other words, should this document be valid? And if it is valid, what do we consider the type of $var in field(arg: $var)?

query Example1($var: String!) {
  ...frag(var: $var)
}
fragment frag($var: String) on Query {
  field(arg: $var)
}

If we allow variable names and types to be overridden in fragments, we remove the dependency between fragment and query variable definitions, and could also allow fragments to be used multiple times in different parts of the query with different variables (what I described above).

[mjmahone]

@ivome unfortunately, I know of at least two GraphQL implementations that could be made to support this more-restricted version of the spec change, but could not, without GraphQL response-format changes, be made to support the looser version you're suggesting.

While I recognize that's an implementation detail, I think it would be a bit of a tragedy if the first version of this new syntax could have enabled clients to be spec-compliant in full support of fragment variables, but because we didn't include an extra validation rule, cannot be.

Basically: I'd rather allow GraphQL implementations to support more than the spec by ignoring certain validation rules, than prevent existing spec-compliant implementations from supporting a future spec because we didn't include enough validation rules. Once we're at the point of fragment variables existing, in a very restricted format, in the spec, it becomes easier to provide a pathway for those other implementations to iterate and drop some validation rules that we no longer want to require.

[netlify[bot]]

✅ Deploy Preview for graphql-spec-draft ready!

Name	Link
🔨 Latest commit	c988b54afc72a53f403bafe24c68df0ab6ec8abc
🔍 Latest deploy log	https://app.netlify.com/sites/graphql-spec-draft/deploys/63bf250c03aa0800092b13bd
😎 Deploy Preview	https://deploy-preview-865--graphql-spec-draft.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[AnthonyMDev]

I'm really excited for this to make it across the finish line! Great work @mjmahone!

I have read all of the discussions above and in the graphql-js PR regarding the fragment argument uniqueness rule, and I agree with @mjmahone that allowing multiple fragment spreads of the same fragment with different arguments on the same object, would require response format changes and a lot more work to implement. But I'm not clear why are aren't allowing them on different objects?

This would be problematic:

query Query($large: Int!, $small: Int!) {
  user {
    ...ProfileImage(size: $large)
    ...ProfileImage(size: $small)     
  }
}

fragment ProfileImage($size: Int!) on User {
  imageUrl(size: $size)
}

But, unless I have missed something, it looks like this example, given by @ivome, would also be invalid:

query Query($large: Int!, $small: Int!) {
  user {
    ...ProfileImage(size: $large)
     friends {
       ...ProfileImage(size: $small)
     }
  }
}

I'm not sure why that is the case. Fulfilling the fragment on different objects should not cause any issues that I have been able to think of so far. The spec already discusses the concept of different objects not needed fields to merge. This should not require the response format to need any changes. I don't have any reason to believe this makes supporting this new feature in other libraries significantly harder.

@mjmahone The example you give in this comment would be invalid, because there would be a field merging collision in https://spec.graphql.org/June2018/#sec-Field-Selection-Merging, as @ivome stated. As long as field merging is still valid, I don't see why this should be disallowed.

---

This operation should still be invalid if the fragment is defined in a way that would cause a field merging violation:

query Query($large: Int!, $small: Int!) {
  user {
    ...ProfileImages(size: $large)
    friends {
      ...ProfileImages(size: $small)     
    }
}

fragment ProfileImages($size: Int!) on User {
  imageUrl(size: $size)
  friends {
    imageUrl(size: $size)
  }
}

This would be invalid because when completing field merging the query.user.friends.imageUrl(size:) field would have two different variable values. But this should be part of field merging validation.

---

Am I mistaken, and this does work the way I'm hoping, or are there other counterexamples that make this problematic?

[mjmahone]

Ah @AnthonyMDev the spec/graphql-JS changes as they exist TODAY would allow merging fragment spreads with different arguments at different levels of the response. It reuses the field merging logic.

My comment from 2021 is outdated! How you want it to work is also how I want it to work. But thanks for the example: I will add a test for the example where the field merging needs to fail despite spreads being on different objects.

[AnthonyMDev]

That's great news! Thanks for all of this. Can't wait to be able to start using this in Apollo!

[mjmahone]

Pulled RFC doc into https://github.com/graphql/graphql-wg/pull/1229 (moved to graphql-wg where RFCs now live).

Note CLA issue was just due to missing an email in my github account. Check should pass on next upload.

[mjmahone]

Closing in favor of https://github.com/graphql/graphql-spec/pull/1010, as this PR carries a lot of historical baggage that new reviewers don't necessarily need, and has caused confusion.