✅ 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.

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

@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 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

@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?

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?

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).

@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.

@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

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

@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