-
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 and stream directives #2319
add support for defer and stream directives #2319
Conversation
2573d53
to
8d3dfac
Compare
src/execution/execute.js
Outdated
@@ -114,6 +118,7 @@ export type ExecutionContext = {| | |||
export type ExecutionResult = {| | |||
errors?: $ReadOnlyArray<GraphQLError>, | |||
data?: ObjMap<mixed> | null, | |||
patches?: AsyncIterable<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.
The result of execute
(and also the main graphql
function) has been modified to return a patches
field, which is an AsyncIterable
. We chose not to change the result of execute
to something like AsyncIterator<AsyncExecutionResult> | Promise<ExecutionResult>
as that would cause type errors for existing consumers of these functions, even if they do not enable these features in their schema.
352d464
to
7f8e2f0
Compare
If anyone wants to try this out locally, I've published these changes to npm under the package |
If you already have
to alias |
} | ||
} | ||
fragment FriendsName on Character { | ||
friends @stream(label: "nameLabel", initial_count: 1) { |
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.
At Facebook, we actually invalidate a query that looks like this. In practice, this is more often a mistake developers make rather than an intentional decision.
To allow developers to create multiple streams on the same field, or to both stream and not stream, we take advantage of the "alias" feature in GraphQL and do something like this:
friendsWithName: friends @stream(label: "namedLabel", initial_count: 1)
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.
@Keweiqu I updated the PR to add a validation rule that forbids streaming on the same field unless the label and initial_count are the same. Does this more closely match the implementation at Facebook?
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.
@robrichard Thanks for adding the validation rule. I do think this validation is universally beneficial no matter which infrastructure developers are working with.
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.
@lilianammmatos thank you so much for driving this effort of adding reference implementations of @defer and @stream for graphql-js! I worked on @defer and @stream Facebook internal implementations, offering some suggestions here based on our experience with this feature.
src/type/dispatcher.js
Outdated
data?: mixed | null, | ||
path: $ReadOnlyArray<string | number>, | ||
label: string, | ||
... |
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.
Relay actually relies on the AsyncIterator to inform whether the current payload is the last payload or not to change its state to "loading_incremental" to "loading_final". If the payload is the last one, the payload will include is_final: true
in the extensions
field.
Because we only insert "is_final" when the payload is the last one globally, there are several edge cases we should consider and test out to make sure that the dispatcher returns correctly. If a query contains multiple @defers and @streams, we don't add "is_final" per stream/defer label. If a query contains a nested @defer inside a @stream
friends @stream(label: "friendList", initial_count: 1) {
nodes {
... on Friend @defer(label: "friendNode") {
name
}
}
}
We insert "is_final" for the correct last payload.
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.
@Keweiqu thanks for the feedback! This is very helpful information. Do you think is_final
in the extensions field is something that needs to be part of the spec for this feature, or is that more of a network implementation detail? In our testing, the patches were sent over a streaming HTTP connection. We were able to signal to Relay that there are no more patches by detecting the HTTP connection has been closed.
I will add some more tests to make sure the AsyncIterator
is closed correctly for queries that contain multiple @stream
s and @defer
s.
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.
@robrichard good question. For the purpose of this PR, my inclination is to aim for only the absolutely essential features, because it's easier to add than to remove.
If we can signal to Relay through detecting a closed HTTP connection without relying on the "extension" field, and @stream/@defer proves to work fine. Let's start with it as the initial step.
We plan to initiate the discussion of evaluating whether to formalize @defer and @stream as a spec and how to best go about it in the next graphql working group meeting. From there on, we will have more information on whether is_final
will be beneficial to the open source community at large. It will also help inform whether it's a good idea to include it in the reference implementation of graphql-js.
@lilianammmatos Thanks for pushing this functionality forward 👍
Correct, here is document describing how new features are accepted into spec:
So the biggest blocker here is the absence of champion for this feature:
Looking at the amount of work done in this PR would be great to have you as a champion for this feature. If so, good next step would be to write a sketch of RFC document similar to the one that we had for Subscriptions: https://github.com/graphql/graphql-spec/blob/master/rfcs/Subscriptions.md And we can discuss it on the Working Group meeting if you would be able to join the call on Jan 9th 5 PM UTC: @Keweiqu Big thanks for sharing your expertise and reviewing this PR. |
@lilianammmatos @robrichard @IvanGoncharov this mode of patch resolution causes a performance deterioration in the execution of the queries because if the same field is requested in 'n' fragment it is resolved 'n' times. You can easily test it using the test case I created: This problem is also present in the @stream directive, and it is for this reason that I have not proposed its integration. |
@IvanGoncharov I do have time to champion this! I'll start working on a sketch of an RFC document and create a pull request against I'll add myself and my colleague @robrichard to the agenda. @Keweiqu thank you very much for your input! |
c29e1e3
to
38580f5
Compare
fc70b69
to
91e9366
Compare
Would it be worth talking about how we expect @defer and @stream to work with a websocket client? Over the https://github.com/apollographql/subscriptions-transport-ws spec might be a good place to start. I couldn't find anything online based on a preliminary search. I can take a stab at putting some notes together if there isn't something already :) |
@coco98 while |
@robrichard Thanks! I just meant that it would be nice to have support over websockets as well. I think this would be super convenient if one is using something like apollo client exclusively via the websocket transport. Considering that the current spec subscriptions-transport-ws supports queries/subscriptions/mutations it might be nice to add some information on what support for defer/stream would look like too. Also, this would kind of play well with the idea that GraphQL is a transport independent spec :) |
Hi @coco98! Agreed that this should remain transport independent. As far as I know, there hasn’t been any discussion of how to adopt |
isFinal relay implementation facebook/relay@78d20cc |
5226175
to
fb24857
Compare
Out of curiosity, has there been any discussion around accepting an optional "priority" in
In our current scenario, I don't actually distinguish between priority 1 and 2 data, since doing so hasn't been ergonomic before Has this kind of functionality been considered internally? |
Hi @mike-marcacci - graphql/graphql-spec#691 has been opened to discuss optimistic preloading. |
can we resync this with graphql v15 https://github.com/graphql/graphql-js/releases/tag/v15.0.0? |
so excited about this! let me know if there’s anything i need to do from the LSP/graphiql side. there is a bit of logic for existing built-in directives |
You probably always want to use AsyncIterableIterator as it allows iterating over it using a async generator function ( There is currently an issue with TypeScript typings and Symbol polyfills (leebyron/iterall#49). Because of that people type their I am also very excited about this as returning |
can we merge and release this as graphql@experimental as react@experimental, relay@experimental ? |
b0e8555
to
d5f0ca5
Compare
@acao We have not had a chance to look at graphiql support for defer/stream yet. It would be great if you have some time to see what's needed get it working. @sibelius we are working with @IvanGoncharov to publish this as an experimental tag on graphql-js after the next version of graphql-js is published. |
src/execution/execute.d.ts
Outdated
export function execute(args: ExecutionArgs): PromiseOrValue<ExecutionResult>; | ||
export function execute( | ||
args: ExecutionArgs, | ||
): PromiseOrValue<ExecutionResult | AsyncIterable<AsyncExecutionResult>>; |
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.
AsyncIterableIterator might be the better return type as it can easily be looped over with a for await of loop
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.
Updated to AsyncIterableIterator
275948a
to
780128d
Compare
so I yarn linked this and the what works transparently
as most of us know, because we're using graphql@15, the introspection schema gives us plenty of information about directives and their arguments. previously, in graphql@14 era, the language service interface and parser had some baked-in spec directives logic because introspection didn't deal with fragments then what doesn't work out of the box? as expected, I'm getting raw multipart responses as per the relay spec, so we might need to implement fetch-multipart-graphql to parse the responses. also, pinging @Urigo because we may have to consider this for the subscriptions fetcher, and supporting deferred schema graphiql PR forthcoming when I next get a chance, unless someone can beat me to it, haha! |
Is there any consensus or discussion ongoing about where to implement the initial data + patch merging on the GraphQL API consumer? Should the fetcher/network (e.g. apollo-link/relay-fetcher) layer always resolve with a ExecutionResult that is built from the initial result and the applied patches that is then consumed by the cache/store/layer or should the client/cache layer (e.g apollo-cache) be able to apply the sent patches to the cache directly? Currently, I guess the network layer is the way to go; but doing it on the cache/store layer could make some optimizations possible. |
@n1ru4l looks like this implementation by the authors has a prescribed pattern for consumers as such, and it works with both apollo server and express-graphql (as per @robrichard 's notes in the express-graphql PR). Thanks @lilianammmatos and rob for building out such a helpful ecosystem and excellent PRs and documentation for this effort! Was very easy to get caught up on the latest details, and to get everything working. I'm thinking that, as with subscriptions, we will make this an opt-in feature via the if that way, if people want to implement I created a discussion in GraphiQL about the GraphiQL implementation, if anyone is interested, would love to hear any feedback or to see folks propose any other/additional ideas they have. it's mostly just a discussion about abstractions around the (to note, apollo studio and i'm guessing a few other graphql IDEs support this feature already) |
Co-authored-by: Ivan Goncharov <ivan.goncharov.ua@gmail.com> support async benchmark tests add benchmark tests for list fields add test for error from completeValue in AsyncIterable resolver change execute implementation for async iterable resolvers correctly handle promises returned by completeValue in async iterable resovlers
780128d
to
54cb43c
Compare
This branch is published to npm as version |
I also created https://github.com/n1ru4l/graphql-bleeding-edge-playground for those that want to have a jump start without having to do all the boilerplate themselves |
Anyone have working examples of the experimental branch using apollo graphql server? |
@AndreasHald I think it is not possible without manual patching the apollo-server-core library (until it is properly implemented). |
@n1ru4l dang :( probably got a while to go then. thanks for the quick reply :) |
Looking forward to this one! I'm seeing it on GQL libs for Java, Scala and Ruby now too. 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. |
@n1ru4l Relay implements support for |
(moving this comment to #2848) |
The implementation in this pull request is a reference implementation of the specification proposal outlined in graphql/graphql-spec#742. It is no longer compatible with with the
@defer
and@stream
implementation in Relay.At 1stdibs, our GraphQL server is built on top of Apollo Server. We were first introduced to the concept of the
@defer
directive through an Apollo blog post detailing an experimental implementation. We attempted to resurrect this initiative by creating an up-to-date pull request (in hopes of that getting merged); however, it became clear to us over time that support for a@defer
directive would occur only if it would make its way into the official GraphQL spec. There has been discussion on this issue to add@defer
to the spec, but it seems it's still in its infancy!Since then, Relay has added support for
@defer
and@stream
, but there are no open source GraphQL servers that support its implementation.This pull request is a proposal for a Relay compatible
@defer
and@stream
directive. The main distinction between the Relay implementation of@defer
and that of Apollo's is that the@defer
directive is supported on a fragment spread and inline fragment instead of on an individual field.Furthermore, this pull request is implemented according to our understanding of Relay’s support for
@defer
and@stream
. We outlined a few examples here: https://gist.github.com/robrichard/f563fd272f65bdbe8742764f1a149b2bThis is a feature we are very passionate about at 1stdibs and would be interested in working with contributors in any way possible!
Sketch RFC: https://github.com/graphql/graphql-spec/blob/master/rfcs/DeferStream.md