Reference implementation of defer and stream spec

[robrichard]

Continued from #2839

This is the reference impementation of the defer/stream spec proposal. The corresponding PR for spec changes are here: graphql/graphql-spec#742

Please limit comments on this PR to code review of this implementation. Discussion on the defer/stream spec is located in a dedicated repo: https://github.com/robrichard/defer-stream-wg/discussions

[netlify]

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Name	Link
🔨 Latest commit	ee0ea6c
🔍 Latest deploy log	https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/63081fa2f330ca0008a48404
😎 Deploy Preview	https://deploy-preview-3659--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[github-actions]

Hi @robrichard, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

• @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
• @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

[github-actions]

@github-actions publish-pr-on-npm

@robrichard The latest changes of this PR are available on NPM as
graphql@17.0.0-alpha.1.canary.pr.3659.cef660554446d49cec9a0958afb9690dd0b19193
Note: no gurantees provided so please use your own discretion.

Also you can depend on latest version built from this PR:
npm install --save graphql@canary-pr-3659

[github-actions]

@github-actions publish-pr-on-npm

@glasser The latest changes of this PR are available on NPM as
graphql@17.0.0-alpha.1.canary.pr.3659.735abf5edacd99b712ddb40d89bd8b213640eb07
Note: no gurantees provided so please use your own discretion.

Also you can depend on latest version built from this PR:
npm install --save graphql@canary-pr-3659

[glasser]

@robrichard @IvanGoncharov I'm finding the types a bit hard to follow here.

So execute can either return (perhaps via Promise) a single ExecutionResult or an async generator of AsyncExecutionResults. That might make me think that the former case happens when incremental delivery is not involved, and the latter happens with incremental delivery.

However, ExecutionResult itself has hasNext and incremental so I guess that's not true. But maybe if you get an ExecutionResult directly from execute it won't have those fields set?

AsyncExecutionResult can be ExecutionResult or SubsequentExecutionResult. The difference is that only ExecutionResult has errors and data; SubsequentExecutionResult has those nested inside incremental.

But I don't get why ExecutionResult can have errors and data both at the top level and nested inside incremental.

I also don't get why ExecutionResult and SubsequentExecutionResult have extension both at the top level and nested inside incremental.

This would make a lot more sense to me if there was the "non incremental delivery" return value that only has data/errors/extensions at the top level, and the "incremental delivery" return value that only has data/errors/extensions nested inside "incremental". Am I missing something?

I'll take a look at the source too to see if that helps me understand it better.

EDITED FOLLOWUP

OK, it looks like the reason that all the fields of AsyncExecutionResult are also on ExecutionResult is related to the use of flattenAsyncIterable in mapSourceToResponse, a subscription-specific function that I don't really understand. This seems like it ought to hopefully be avoidable?

[glasser]

I tried to simplify the types as described above. #3703 is what I came up with.

A lot of the complexity comes from the incremental field; this is a pretty recent addition, right? I'm not sure I really understand why it exists rather than the fields on it going directly on multiple top-level messages. Something about being able to deliver multiple deferred chunks at once? Does incremental get actually written in the JSON on the wire?

[glasser]

Having tried to implement consuming in Apollo Server, I'm increasingly convinced that (assuming we don't want the first message to have an incremental field, which seems to be the consensus) we should make execute (or its replacement) always return a "first execution result" (which should never have an incremental field), and then if anything is deferred/streamed, also return an async iterator of "subsequent execution results" (which should always have an incremental field). That works a lot better from the consumer side than the current async iterator which is "never empty and the first one has a different type from the rest". I would also much rather have a nice strongly typed optional field with the subsequent result iterator than have to probe a result to figure out what kind it is (basically requiring me to copy and paste isAsyncIterator into my codebase).

[glasser]

One option is to just avoid async iterators entirely. @benjamn privately suggested to me the idea that each chunk just contains a next(): Promise<Chunk>; there's no complexity about parallel calls here because each next() call on the same chunk always would return the same next chunk. You'd need some other explicit close() or cancel() or whatever call. The main advantage of async iterators to me is just that it's how graphql-js already works for subscriptions.

Fortunately this is purely a question about how to handle this in graphql-js, not something that should block the overall of the spec update from merging. I think sticking with the same API as subscriptions makes sense, and graphql-js could consider later creating a second new API (both could be supported) for following these streams if we want.

[glasser]

(BTW my basic opinion about concurrent next/returns is that if we claim to implement AsyncIterator then we should do so correctly, but if we eventually decide a simpler API is good enough for our use case then that's good too. For the specific case of defer/stream results, the ordering between chunks is quite important — the chunks are designed to be applied in order. So the ability to wait on multiple next calls in parallel (or to use return for anything other than cleanup) is not really important if you ask me.)

[michaelstaib]

@robrichard is the new items name for the result already implemented? Went through the tests but did not find any result with the items field for stream results.

[robrichard]

@michaelstaib yes items is implemented. See the tests in stream-test.ts: https://github.com/graphql/graphql-js/blob/defer-stream/src/execution/__tests__/stream-test.ts#L140

[michaelstaib]

Ah, overlooked this one. Thanks for pointing this one out.

[calvincestari]

@robrichard - I know this was a while ago but is it correct that GraphQLDeferDirective and GraphQLStreamDirective were not added to the specifiedDirectives list? For schemas that want to use those directives should they manually add them to the config/schema directive lists?

Update: https://github.com/graphql/graphql-js/blob/main/website/docs/tutorials/defer-stream.md