Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Fragment Arguments (parameterized fragments) for GraphQL #865

Closed
wants to merge 7 commits into from

Conversation

mjmahone
Copy link
Contributor

@mjmahone mjmahone commented May 5, 2021

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

@mjmahone mjmahone changed the title RFC: Fragment Arguments RFC: Fragment Arguments for Client GraphQL May 5, 2021
@andimarek
Copy link
Contributor

@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
Copy link
Contributor Author

mjmahone commented May 6, 2021

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
Copy link
Member

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

@IvanGoncharov IvanGoncharov added the 📣 RFC document PR creates or changes document inside "rfc" folder label May 13, 2021
@mjmahone
Copy link
Contributor Author

mjmahone commented Jun 2, 2021

I actually think the executor implementation is not that difficult: graphql/graphql-js#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.

rfcs/FragmentArguments.md Outdated Show resolved Hide resolved
@ivome
Copy link

ivome commented Jun 9, 2021

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
Copy link
Contributor Author

@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
Copy link

ivome commented Jun 10, 2021

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
Copy link
Contributor Author

@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
Copy link

netlify bot commented Jan 2, 2023

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit c988b54
🔍 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...

QR Code

Use your smartphone camera to open QR code link.

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

@mjmahone mjmahone changed the title RFC: Fragment Arguments for Client GraphQL RFC: Fragment Arguments (parameterized fragments) for GraphQL Jan 3, 2023
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
Copy link
Contributor

@rivantsov rivantsov Jan 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Operation-defined variables ... ... within the operation

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reused (no dash)

}
```

In this case, the `user` will have a larger `profilePic` than those found in the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure but I don't think you need comma after 'In this case'


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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

operation-defined

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:`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you need colon after size?


FragmentArgumentDefinition : Description? Variable : Type DefaultValue?
Directives[Const]?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we introduce FragmentArgumentDefinition instead of reusing VariableDefinition just because we want to add Description as well? How about simply adding Description to Variable definition - let's discuss it. If Description makes sense here then it makes sense in Operation variable definition (even more I think)

Copy link
Contributor Author

@mjmahone mjmahone Jan 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried creating an implementation where we reuse VariableDefinition, and it turned out to be more clunky than this. Fragment argument definitions need to be described everywhere as being argument definitions, not as defining a variable. It causes confusion when referring to a fragment argument's variable definition, IMO. Furthermore, in the actual implementation in graphql-js, the AST refers to arguments in the same way that a field or directive has arguments. When those arguments are defined via variable definitions, you end up over-visiting the VariableDefinition values and needing to check the context of those definitions more than if you have a unique AST value for fragment arguments.

If I could, I would have reused InputValueDefinition: logically, fragment argument definitions produce the same kind of thing in the language as field argument definitions, directive argument definitions, and input object field definitions. Ideally, I would have had FragmentArgumentDefinition : $ InputValueDefinition. This doesn't work, though, as InputValueDefinition is defined as InputValueDefinition : Description? Name : Type DefaultValue? Directives[Const]?.

I could be swayed that it makes sense to reuse VariableDefinition, so long as we don't need to update the text to describe them as anything other than argument definitions in the same way that field arguments are argument definitions in the Spec description. However, another advantage to separating the two is that we probably do not want a directive that is allowed on VARIABLE_DEFINITION to automatically be allowed on FRAGMENT_ARGUMENT_DEFINITION and vice-versa.

On the Description front, agree it probably makes sense to add Description to Operation VariableDefinition, but even so I think FragmentArgumentDefinition makes sense to separate as it is defining an input as opposed to VariableDefinition, which defines a globally available value.

```

the response will have two conflicting versions of the `doesKnowCommand` that
cannot merge.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in what sense 'cannot merge'? I can write implementation that merges them. The fields have different values I guess. Would be better 'should not merge'?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the same sense as above,

However, the field responses must be shapes which can be merged

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again 'short circuit' but now without dash.

```

the following is invalid since `command` is not defined on `DogCommand`.
the following is invalid since `command:` is not defined on
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

command: -> command argument


```graphql counter-example
fragment invalidArgName on Dog {
doesKnowCommand(command: CLEAN_UP_HOUSE)
}
```

and this is also invalid as `$dogCommand` is not defined on fragment
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$dogCommand -> dogCommand argument

```

the following is invalid since `command` is not defined on `DogCommand`.
the following is invalid since `command:` is not defined on
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Capital T at sentence start

@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

operation-defined (dash)


because
{$atOtherHomes} is only referenced in a fragment that defines it as a
locally scoped argument, the operation defined {$atOtherHomes}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

operation-defined

**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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they're - bad style to use these speech shortcuts in documents, change to 'they are'


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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe simply 'fragment does not use the argument' ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably. I'll change it but we're not explaining the "local scope" well (as a variable being used by an operation includes any recursive fragments, but in this case use means excluding recursive fragments).

fragment does not contain a variable with the same name as the argument, then
the argument is superfluous.

For example the following is invalid:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}
```

This document is invalid because even though `fragmentArgUnused` is spread with
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'is A spread' (currently 'spread' sounds like a verb)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under "Fragment Spread is Possible", spread is used as a verb:

Fragments are declared on a type and will only apply when the runtime object type matches the type condition. They also are spread within the context of a parent type.

I think we use spread as a verb explicitly: spreading is the action that you take when applying a fragment spread.

Copy link
Contributor

@rivantsov rivantsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, with few comments

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what short-circuit means

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to update the algorithm to explicitly fail if two spreads with the same name have different arguments, rather than using a "short circuit" (stop early) option.

@AnthonyMDev
Copy link

AnthonyMDev commented Jan 6, 2023

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
Copy link
Contributor Author

mjmahone commented Jan 6, 2023

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
Copy link

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

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't work really well either. If you do

fragment Bar($x: Int = null) { number(x: $x) }

then what would $x be in ...Bar? Would it be null or undefined? To be consistent with other default values, I'd expect it to be null but then there's no way to pass undefined (no value).

Not sure what a good solution would be. Maybe add a undefined keyword and allow a subset of union types...

fragment Bar($x: Int | undefined)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think allowing undefined variables was a mistake :)

However, the reason this version is labelled as an alternative is because it would try to fix that mistake in one place (fragment arguments) and not elsewhere (operation variables, field/directive arguments, input objects). At the very least we'd want to unify with either operation variables or (field/directive arguments and input objects). Since fixing the mistake of undefined inputs is outside the scope of this feature, I'm opting to keep the same behavior we already have (namely, unset gets resolved to either the fragment defined argument default value, or, if there is no default and the variable is nullable, resolves to null or the field defined argument default value).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think allowing undefined variables was a mistake :)

Gotcha 👍 I think I agree. I cannot think of a use case where a undefined variable or field argument is used (but would be interested to learn about them if any).

For the sake of consistency, I'd rather keep the same behaviour as operation variables though. It took me enough time to understand how it was working that I don't want to learn something new for fragments 😅

@mjmahone
Copy link
Contributor Author

mjmahone commented Jan 11, 2023

Pulled RFC doc into graphql/graphql-wg#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
Copy link
Contributor Author

Closing in favor of #1010, as this PR carries a lot of historical baggage that new reviewers don't necessarily need, and has caused confusion.

@mjmahone mjmahone closed this Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💡 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md) 📣 RFC document PR creates or changes document inside "rfc" folder
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants