-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add support for @defer directive #1287
Conversation
0d15d81
to
6c1a1f1
Compare
Hmm, I think this could be a bit confusing, since the calling code now needs to potentially deal with two different types. Could this be just one, or maybe a separate function that supports defer? |
Also, do we want to combine the interface with subscriptions in any way, since both are ways to get an initial result and then updates? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial feedback
``` | ||
// Initial Response | ||
{ | ||
"asset": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would have a data
field right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
- Proposed format for `ExecutionPatchResult` | ||
```graphql | ||
{ | ||
path: [string]! |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
||
#### Apollo Server: | ||
|
||
In order to support @defer, there are significant changes to be made to the execution phase if GraphQL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*of
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
||
#### Restrictions on @defer usage: | ||
|
||
- _Mutations:_ already execute serially, so there does not seem to be any use case for @defer. We should throw an error if @defer is applied to the root mutation type. However, @defer should apply normally for the selection set on a mutation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, @defer should apply normally for the selection set on a mutation.
I think we maybe decided that defer shouldn't be allowed anywhere in a mutation?
Also, I think that should also apply to subscriptions, at least at first to simplify things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes! Because there is no analog to watchQuery on the client! Can do!
@@ -0,0 +1,1630 @@ | |||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that makes this file harder to review is that it's hard to know what comes from graphql.js and what is new. Can you point me to some of the most important functions to review?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely. Let me strip out all the utility functions and leave only those that changed.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
a44fc1f
to
6dc307e
Compare
|
||
#### A Better Way | ||
|
||
_@defer_ provides a way for us to mark fields in our schema to _not_ wait on, because they might be: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should mention that @defer
has been used inside Facebook and link to Lee's talk. Maybe we can say something like: the concept of @defer
has generated a lot of interest ever since it was first talked about in 2016, but until now there was no way for people to actually use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added this under A Better Approach
in the README.md. Thanks for pointing out!
*/ | ||
export interface DeferredExecutionResult { | ||
initialResult: ExecutionResult; | ||
deferredPatchesObservable: Observable<ExecutionPatchResult>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm honestly still confused by the difference between Observable
and async iterators, but since async iterators are already part of graphql-js
for supporting subscriptions, and async iterators recently reached stage 4 and will be part of ES2018, should we consider using them instead? See http://2ality.com/2016/10/asynchronous-iteration.html.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Observable
is still at stage 1 and doesn't seem to be going anywhere: https://github.com/tc39/proposal-observable
Maybe we can ask @benjamn what his thoughts on this are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I'm a little afraid of taking a dependency on RxJS because we want the bundle size on the server to stay as small as possible for edge execution. And V8 may get more optimized execution for async iterators (no idea if that makes sense or what it would look like).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out async iterators are already available natively in Node 10 and recent versions of Chrome! They require some getting used to, but for-await-of and async generators are pretty powerful. See http://2ality.com/2018/04/async-iter-nodejs.html for examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out Martijn! As per James' comments, I think this will require some more thinking about the pros and cons of both approaches. In the meantime, I will look at the subscriptions implementation to get a better understanding.
} | ||
``` | ||
|
||
Instead of holding the GraphQL response and page rendering until the entire query is resolved, @defer tells Apollo Server to return a partial query response if the deferred fields are not ready yet. In the example above, we see how it may be used to remove `progress` and `comments` from the intial query response. Apollo Server would then take care of resolving the rest of the deferred fields in the background, and stream them to the client when they are ready. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also support nested @defer
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@clarencenpy this is a great start! I left some comments on specifics but there are two main things I think we need to change:
-
Why is this another package? I would have expected it to be in apollo-server-core. Lets be really careful about adding a ton of packages here as it increases complexity to manage
-
I think the results need to include the field that is defered with a null value. Since we are limiting usage to nullable types, this won't impact type generation tools and will require the least amount of work for clients. It also makes debugging easier since you don't have to determine if there was a problem with the sent operation, but rather can see if the initial result has the data here. Additionally, it makes it easier to work with inside UI components
if (observer) { | ||
observer.next(patch); | ||
} else { | ||
return new Observable<ExecutionPatchResult>(observer => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nit, but lets do return Observable.of(path)
instead
@@ -0,0 +1,1630 @@ | |||
/** | |||
* Copyright (c) 2015-present, Facebook, Inc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets reference the original here but adjust the license with giving credit
query: queryType, | ||
types: [humanType, droidType], | ||
directives: [GraphQLDeferDirective], | ||
// TODO: Eventually bake in @defer as a standard directive and do validation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should merge this is until that is done (baked in)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! Instead of modifying the validation phase of graphql.js, I think a more minimal approach would be to modify the call to GraphQLSchema's constructor within AS2, to pass in the additional schema directives.
@@ -0,0 +1,337 @@ | |||
/** | |||
* Copyright (c) 2015-present, Facebook, Inc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove and give credit in the LICENSE file
@@ -0,0 +1,498 @@ | |||
/** | |||
* Copyright (c) 2015-present, Facebook, Inc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment
|
||
#### Who needs this the most? | ||
|
||
- Media companies, banks, other data-heavy enterprise applications. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is needed. You did a great job outlining the benefits above, lets not count usage of this out for anyone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right there! @defer is for everyone 👍
|
||
In order to support @defer, there are significant changes to be made to the execution phase if GraphQL. | ||
|
||
- Roll our own derivative of graphql.js execution that supports deferred responses and streaming patches using observables. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would characterize this as extending graphql-js execution to support deferred responses. I think its important for us to compare the value of async itterable (which already has support for subscriptions) and observables. I like observables but want to have strong reason for adding another data type within the system.
However, since we don't expose this data type, it may not even be worth mentioning and instead keep it as a changeable implementation detail. I do think that we may one day want to allow returning of an async iterable or observable for control of something like stream / live
so lets give this some careful thought.
@evans I know you have done some thinking here, care to chime in?
#### Apollo Client: | ||
|
||
- No changes required | ||
- Initial implementation of @defer support should come as a Apollo Link. Reads from a socket connection or some other event stream, keeps the partial response in memory, merging patches as they come and pushing it through the link stack. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should do it this way. @defer
will need special traits for reading / bypassing the cache so I think we should build it into Apollo Client core. Its also important to make this a seamless experience for people and adding another link is not ideal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would love to chat with you more about the changes required on the client side. My original thinking was to minimize the amount of changes needed on the client, but we can and should strive for a seamless experience!
|
||
#### Transport: | ||
|
||
- @defer requires a transport layer that is able to stream or push data to the client, like websockets or server side events. If the transport layer does not support this, Apollo Server should fallback to normal execution and ignore @defer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and warn in the console or something if falling back
``` | ||
|
||
- The reason that this behavior is useful is because some fields can incur high bandwidth to transfer, slowing down initial load. | ||
- We could allow the user to control this behavior, by taking an optional waitFor argument: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is really interesting and we probably also want some kind of timeout so a socket doesn't get stuck for some reason. @peggyrayzis this could have interesting supsense tie ins
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed! It would be SOO cool to sync the same waitFor
times throughout the presentation and data loading layer!
Replying to @jbaxleyiii
I'd definitely hope to merge it with apollo-server-core!
That makes sense. Made the changes! |
e4513d7
to
6965c25
Compare
d0cefaa
to
ed8e06d
Compare
5c783ee
to
064b86d
Compare
So excited for this! 😻 Awesome work! |
064b86d
to
668634a
Compare
What's the status of this? I have a custom defer solution that I would like to ditch if possible, but it seems like this PR has stalled. Is there anything I can do to help? |
@clarencenpy I'm so excited for this feature! 🙌 Any updates on this PR getting merged? It seems to have fizzled out a bit. Let me know if I can be of any assistance. |
having a custom execute function for graphql will make it hard to keep it sync with execute from graphql-js code should we try a RFC on this one to get this custom execute on graphql-js? or maybe break execute in some functions to be able to create new executions easily |
Is there any updates on |
Status? |
Any updates here? Looks like there hasn't been any activity for a year |
Please please please consider keeping going on this.... This solves one of my biggest gripes with GQL - your response performance is limited to the performance characteristics of the slowest resolver dependency unless you issue multiple queries which really limits the ability of GQL to serve as a near perfect abstraction over legacy APIs etc. I have 3 queries to paint a page now to keep things performing and it's a real mess... they are aptly named |
The implementation of |
@glasser any updates on this? 🙏 |
We didn't hit this year as you noticed, but yes, we have plans. We're working on our 2022 roadmap at the moment and there will be details on defer/stream in there. |
@glasser is there a tracking issue for Apollo Server support of this feature? |
Posted the roadmap today: https://github.com/apollographql/apollo-server/blob/main/ROADMAP.md Will be breaking this up into individual issues shortly. Note that we are not committing to definitely supporting defer in 4.0.0, but part of the goal of the refactor is to make defer easy to implement instead of the challenge it is today due to unnecessarily tight coupling between the core Apollo Server logic and the web framework integrations. |
This PR proposes the addition of @defer support to Apollo Server.
Full details of the proposed spec can be found here.
Summary:
execute.js
fromgraphql.js
. Functions that are untouched have been removed, and exported fromgraphql
directly.execute()
returnsExecutionResult | DeferredExecutionResult
, the latter being if@defer
has been applied to any fields.DeferredExecutionResult
wraps anExecutionResult
containing the initial response, and anAsyncIterable<ExecutionPatchResult>
.Testing:
__tests__/starWarsDefer-test.ts