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

Support @defer directive #2318

Closed
wants to merge 10 commits into from

Conversation

morrys
Copy link

@morrys morrys commented Dec 30, 2019

Hello to all,
this PR allows to support the @defer directive. Although the implementation is mostly complete, the PR is in draft to discuss it together and evaluate its integration.

In the PR I also created tests and I performed the integration tests with Relay, express-graphql & fetch-multipart-graphql with positive results.

In summary how I implemented defer:

  • @defer has two arguments:
    • if (optional, default true) the fragment is defer when true.
    • label (required) used to distinguish deferred fragments
  • @defer currently provides only FRAGMENT_SPREAD & INLINE_FRAGMENT (Relay) as location, but support for FIELD (Apollo) is commented in the PR
    /*
    in order to support defer on field
    const isFieldDefer = shouldDeferNode(exeContext, selection);
    if (deferDirective && !isFieldDefer) {
    */
    if (deferDirective) {
  • I added resolverResult: ResultResolver new property in ExecutionContext which allows the management of the results to be returned at the end of the execution
  • modified execute function now its return type is AsyncIterable<Promise<ExecutionResult> | PromiseOrValue<ExecutionResult>; . Using isAsyncIterable (result) it is possible to distinguish if there are deferred results.
  • fields that are marked as deferred are resolved after the resolution of the previous result. I also implemented an optimization to avoid solving the same field multiple times.

this is a slow motion gif of how it works.
deferred

Let me know if more information is needed.

Thank you
Lorenzo

@wtrocki
Copy link
Contributor

wtrocki commented Dec 30, 2019

Some additional context from Apollo Defer: apollographql/apollo-server#2700

@morrys
Copy link
Author

morrys commented Dec 31, 2019

Looking at the Relay test schema
https://github.com/facebook/relay/blob/5da3be070283c6dcd42774ba33c1590db65fe3c7/packages/relay-test-utils-internal/testschema.graphql#L13-L16 i noticed that defer is also supported for inline_fragment, the last commit support it and add some tests

@morrys
Copy link
Author

morrys commented Dec 31, 2019

In this branch I created support for @stream:
https://github.com/morrys/graphql-js/tree/support-stream-directive

Some checks and optimizations are missing. But the first tests with Relay were positive:
defer-and-stream-directive

@IvanGoncharov
Copy link
Member

@morrys Thanks for your effort 👍
I didn't have enough time to read through actual code but on the surface, it looks like both #2318 and #2319 add very similar functionality especially since both of them Relay compatible.
Or I'm missing something?

About the next steps for merging this PR please see my answer in #2319:
#2319

@morrys @lilianammmatos Can you please review each other PRs and figure out how to collaborate?

@IvanGoncharov IvanGoncharov added the spec RFC Implementation of a proposed change to the GraphQL specification label Jan 4, 2020
@morrys
Copy link
Author

morrys commented Jan 4, 2020

@IvanGoncharov yes the two PR add the same functionality.
Tomorrow I integrate the branch where I implemented the stream and then I dedicate myself to making a review of @lilianammmatos 's PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec RFC Implementation of a proposed change to the GraphQL specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants