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

Proposal: Deferrable and Streamable Directives Arguments #704

Closed
lilianammmatos opened this issue Mar 31, 2020 · 8 comments
Closed

Proposal: Deferrable and Streamable Directives Arguments #704

lilianammmatos opened this issue Mar 31, 2020 · 8 comments

Comments

@lilianammmatos
Copy link
Contributor

lilianammmatos commented Mar 31, 2020

Introduction

Initially referred to as @defer and @stream, this proposal suggests rebranding these directives as @deferrable and @streamable in order to explicitly denote that these directives are hints to the GraphQL server not directions.

@deferrable, a fragment-level directive, and @streamable, a field-level directive, are directives that signal to the server what data the client can handle asynchronously i.e. delivered outside of the initial response. These directives behave as hints to the server that it can choose to ignore and neither defer nor stream the specified fragment.

Therefore, the client should be prepared to handle these fragments and fields in the case that they are delivered with the initial response when either directive is present. The server will never deliver data asynchronously that the client has not specified that it can handle or has explicitly stated that it cannot handle e.g. @streamable(if: false).

Proposal

@deferrable

  • if: Boolean
    • When true fragment may be deferred, if omitted defaults to true.
  • label: String!
    • A unique label across all @deferable and @streamable selections in a query.
    • This label should be used by GraphQL clients to identify the data from patch responses and associate it with the correct fragment.

@streamable

  • if: Boolean

    • When true field may be streamed, if omitted defaults to true.
  • label: String!

    • A unique label across all @deferrable and @streamable selections in a query.
    • This label should be used by GraphQL clients to identify the data from patch responses and associate it with the correct fragments.
  • Options for chunk size argument(s):

    • Option 1
      • initial_size: Int - The chunk size the server should return as part of the initial response. (Potential MVP solution)
    • Option 2
      • initial_size: Int - The chunk size the server should return as part of the initial response.
      • size: Int - The chunk size for asynchronously delivered responses.
    • Option 3
      • size: [Int] - The first element in the array is the initial chunk size. Any subsequent elements correspond to the chunk size of the following chunk where the last element also applies to the size of the remaining chunks as well.
  • Options for server handling of chunk size argument(s):

    • Option A
      • Size arguments are mandatory. If they are specified the server must follow them exactly.
    • Option B
      • Chunk size arguments are minimums. If they are specified, the server must return at least this number of items in the response. The server may return more than the specified amount if it is efficient for the server to do so. If this option is selected, we recommend renaming the arguments to min_initial_size, min_size.
    • Option C
      • Chunk size arguments are hints. If they are specified, the server may choose to ignore them if it is more efficient for the server to send different batch sizes. Clients must be able to handle receiving less data than requested in responses. Clients may also need to batch streamed items client side to avoid rendering "sparse" UIs.

--

Any feedback would be greatly appreciated!

@soneymathew
Copy link

What would be the implications of specifying both directives to the same field?

{
  feed {
    stories @deferable(label:"l1") @streamable(label:"l2") {
      ...storyInfo
    }
  }
}

Suggestion: Would be great if you could add a few real world examples into the RFC

@josephsavona
Copy link
Contributor

josephsavona commented Apr 3, 2020

Thanks for writing this up @lilianammmatos! I think it might help to expand this proposal a bit - not all the way to a fully flushed out RFC, but with a bit more information such as examples and an informal description of what these directives do/ how they work.

@soneymathew Note that @defer can only be applied to fragment spreads and inline fragments, not to fields, and @stream can only be applied to fields. So they can't co-exist on a selection.

Re naming: While yes, technically these directives are hints to the server, the server is generally expected to honor them (clients must be prepared to handle the case in which the server does not honor the hint, but clients can reasonably expect that to be an edge case). As such, I don't believe the longer names (deferrable/streamable) offer significant value here vs the shorter and easier to spell alternatives. I suspect that if the spec ends up choosing the longer names, that client side tooling will just allow people to use the shorter names anyway, compiling it to the longer name transparently.

@lilianammmatos
Copy link
Contributor Author

lilianammmatos commented Apr 3, 2020

Note that @defer can only be applied to fragment spreads and inline fragments, not to fields, and @stream can only be applied to fields. So they can't co-exist on a selection.

This was an oversight! That is how we expect these directives to work. Thanks for catching that.

Re naming ... clients must be prepared to handle the case in which the server does not honor the hint, but clients can reasonably expect that to be an edge case ...

Whether or not that is an edge-case, which I don't think it is and would likely vary across implementations, the client should be prepared for both cases. The longer names explicitly express that intention whereas for the shorter names that intention is implicit and could cause confusion if a field/fragment marked for asynchronous delivery is delivered with the initial payload. There's valid arguments for both naming conventions, it's still something we'll need to decide on.

@josephsavona do you have an opinion on the client's expectations for chunk sizes? We'd appreciate any insight on what options (if any) would be ideal or non-ideal from a GraphQL client perspective.

@soneymathew
Copy link

@soneymathew Note that @defer can only be applied to fragment spreads and inline fragments, not to fields, and @stream can only be applied to fields. So they can't co-exist on a selection.

@josephsavona / @lilianammmatos I will be keen to learn about the rationale behind this restriction/tradeoff. Is it because tagging a fragment as streamable spreads the streaming requirement to all the individual fields within the fragment? Also not able to think of scenario why a field cannot be deferrable and streamable at the same time.

Does this below use case look valid?

{
  feed {
    stories  {
      ...someStoryInfo @deferable(label:"D1")
      likeCount @deferable(label:"D2") @streamable(label:"S1")
    }
  }
}

@AlicanC
Copy link

AlicanC commented Apr 9, 2020

Let's say I'm charting some DATA in my APP, which I fetch with a QUERY from a SERVER using a CLIENT. I notice that there's so much DATA to process, APP lags when the QUERY response arrives. I decide to solve this by telling the CLIENT to stream the data by changing my QUERY to use @streamable. At this moment, both the CLIENT and SERVER supports streaming so everything works perfectly.

Then for some reason the SERVER changes and stops streaming this QUERY. What happens?

  • APP receives DATA all at once and starts to lag
  • CLIENT receives DATA all at once, but still streams it to the APP, so there's no lag

If the CLIENT will behave like the former (or how the CLIENT is going to behave will not be spec'd, which would mean the former can happen), how do I prevent this? Never depend on streaming in the first place and make sure the APP works without it?

@josephsavona
Copy link
Contributor

these directives as @deferrable and @streamable in order to explicitly denote that these directives are hints to the GraphQL server not directions.

I'd like to revisit this point. As @AlicanC noted, the ability to defer/stream parts of a response can have a potentially significant affect on application performance. Developers generally need clear, predictable control over their application's performance. If the server can make arbitrary decisions about which directives it will honor or not on each execution, then developers will not be able to rely on these directives and will be forced to manually recreate the desired behavior to ensure predictable performance.

The @include and @skip directives are good analogues here. Technically speaking, developers must be prepared for the presence or absence of the relevant selections: any given response could either contain or not contain those selections depending on the variable value. But developers need to be able to rely on these directives always taking effect when applied to avoid accidentally regressing application performance or server efficiency (as could happen if, say an expensive/slow field that they intended to skip was accidentally fetched by the server).

So: I'd argue strongly in favor of the shorter @defer and @stream names, but more importantly that these are not just hints, but directives to the server that should be followed.

@maraisr
Copy link
Contributor

maraisr commented Apr 14, 2020

@josephsavona Can i also just interject, and echo your point. Granted this isn't Relay, but the very reason Relay is so good in my opinion, is because of this predictability. Please do shorten the name to @defer and @stream - and make the spec read that it shall be honored, and ERROR if it can't.

@lilianammmatos
Copy link
Contributor Author

We decided on the shorter naming convention, which we've documented in this RFC: https://github.com/graphql/graphql-spec/blob/master/rfcs/DeferStream.md.

Note that the RFC is not final, and we're actively making changes! Thanks everyone. I'll be closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants