defer/stream: split incremental delivery into new entry points #3703

glasser · 2022-08-16T23:37:35Z

This PR changes the execute, subscribe, and graphql APIs on the
defer-stream branch back to having the same API as on main: they can only
produce a single ExecutionResult (or a stream of ExecutionResults for
subscribe) which never has a hasNext field, and do not support incremental
delivery.

Incremental delivery is now provided by the new entry points
experimentalExecuteIncrementally and experimentalSubscribeIncrementally. (We
will remove "experimental" once the proposal has been merged into the GraphQL
spec.)

This will make upgrading to graphql@17 easier, and increases the clarity
of return types.

Use distinct types for "the only result of a single-payload execution",
"the first payload of a multi-payload execution", and "subsequent
payloads of a multi-payload execution", since the data structures are
different. (Multi-payload executions always have at least one payload,
and the structure differs, which is why the new types separate the
initial result from subsequent results.)

Namely, single results have no hasNext field; initial results have hasNext
and may combine data/errors with incremental; and subsequent results have
hasNext and incremental.

Note that with the previous types, you actually had to use a function
with a type guard like isAsyncIterable: you couldn't just write if (Symbol.asyncIterator in result). I think both explicitly separating
the first (differently-typed) element from the rest of the elements
and making it possible to differentiate the types with a simple in
check are improvements. Additionally, somebody using the API in
JavaScript who doesn't realize that there is new functionality available
who is confused that the result no longer has data/errors fields can
just print out the result and see and search for the terms
initialResult and subsequentResults instead of only seeing iterator
internals.

Fix Formatted*Result types to use GraphQLFormattedError rather than
GraphQLError for errors nested inside incremental.

github-actions · 2022-08-16T23:38:11Z

Hi @glasser, 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

glasser · 2022-08-19T05:17:37Z

Updated this PR to be less of an experiment and more of an actual proposal. Updated the PR description too.

I think the c8 ignore lines I added probably show either tests that could exist or actual error handling bugs, tbh.

yaacovCR · 2022-08-19T15:25:36Z

This seems to have another tiny advantage besides typings: If we return a generator that includes/emits the first payload (current behavior) rather than returning the first payload separately (as in this PR), then even if the first payload is produced synchronously, the client will have to await it.

Even if it’s produced asynchronously, with current behavior, the client will have to await the generator (which is not produced until the first payload is) and then again have to await the first payload emitted by the generator.

Perhaps this pattern also makes sense generally for every generator that is known to be non-empty…

Fixes the bug demonstrated in graphql#3709 (which has already been incorporated into the defer-stream branch). This fix is extracted from graphql#3703, which also updates the typing and API around execute. This particular change doesn't affect the API (other than making the `subscribe` return type more honest, as its returned generator could yield AsyncExecutionResult before this change as well).

Fixes the bug demonstrated in graphql#3709 (which has already been incorporated into the defer-stream branch). This fix is extracted from graphql#3703, which also updates the typing and API around execute. This particular change doesn't affect the API, other than making the `subscribe` return type more honest, as its returned generator could yield AsyncExecutionResult before this change as well. (The reason the previous version built is because every ExecutionResult is actually an AsyncExecutionResult; fixing that fact is part of what graphql#3703 does.)

Fixes the bug demonstrated in #3709 (which has already been incorporated into the defer-stream branch). This fix is extracted from #3703, which also updates the typing and API around execute. This particular change doesn't affect the API, other than making the `subscribe` return type more honest, as its returned generator could yield AsyncExecutionResult before this change as well. (The reason the previous version built is because every ExecutionResult is actually an AsyncExecutionResult; fixing that fact is part of what #3703 does.)

glasser · 2022-08-20T18:52:25Z

@robrichard @IvanGoncharov I've rebased this on top of #3659 with the #3710 fix incorporated, and I've updated the PR description to be more informative and less "experimental".

glasser · 2022-08-23T02:43:42Z

After discussion with @IvanGoncharov I've updated this to move incremental delivery out of execute into a new entry point experimentalExecuteIncrementally (which I recognize is not quite aligned with the discussion at graphql/defer-stream-wg#12). This seems like the most appropriate way to let us move forward.

yaacovCR · 2022-08-23T03:31:46Z

src/execution/execute.ts

+    return result;
+  }
+  // Always return a Promise for a consistent API.
+  return Promise.resolve({ singleResult: result });


Question: Is the benefit of always returning a promise definitely worth it?

If the first result is synchronous,
returning the generator could also be synchronous and then we save a tick.

This change actually sort of makes the API inconsistent in another sense as non incremental delivery can be either value or Promise, so maybe shouldn't incremental.

I believe we recently changed in v17 subscribe to allow returning a generator synchronously. Looks like now we r headed in the other direction. Either way, should they be aligned?

@yaacovCR Agree, with can make this function return PromiseOrValue<ExperimentalExecuteIncrementallyResults>

yaacovCR · 2022-08-23T03:34:37Z

src/execution/execute.ts

+ * Helper for the other execute functions. Returns an ExecutionResult
+ * synchronously if all resolvers return non-Promises
+ */
+function executeSyncOrIncrementally(


Do we need this combination?

I agree, with @yaacovCR on that. It's DRY and saves a few lines. But given all the planned changes to execute API, it would be better to just duplicate those lines in both functions.

@yaacovCR Also, it's a non-exported function, just a utility one to be DRY.
But I still think it's somewhat confusing and don't worth the DRY benefits it gives.

robrichard · 2022-08-23T14:11:16Z

src/graphql.ts

-export function graphql(
-  args: GraphQLArgs,
-): Promise<ExecutionResult | AsyncGenerator<AsyncExecutionResult, void, void>> {
+export function graphql(args: GraphQLArgs): Promise<ExecutionResult> {


We don't want to export a version of graphql that supports incremental delivery?

I think this was @IvanGoncharov 's idea but I might be misinterpreting. Probably planning to change when not "experimental"?

@robrichard Yes, @glasser is right.
The idea is to finally merge it into main having an "experiment" prefix to everything.
Changing the prototype of graphql makes it a breaking change, so better to avoid it before we are sure that the shape of the response is final.

So this PR has two things inside:

Stuff @glasser learned by implementing this feature in ApolloServer (changes to execute typings).

Moving everything under the "experiment" prefix and cutting non-essential functionality (e.g. graphql) with the end goal of merging steam/defer to main.

github-actions · 2022-08-23T14:22:03Z

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

glasser · 2022-08-23T19:15:23Z

src/execution/execute.ts

+export interface InitialIncrementalExecutionResult<
+  TData = ObjMap<unknown>,
+  TExtensions = ObjMap<unknown>,
+> extends ExecutionResult<TData, TExtensions> {


This should have incremental as well, according to the spec, even though graphql-js doesn't write that.

github-actions · 2022-08-23T19:23:51Z

@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.3703.9360805de6310b453b76a53431f921b44a76c2f9
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-3703

glasser · 2022-08-23T19:27:19Z

I should apply the same "un-transformation" to subscribe: split it into a v16-esque subscribe and experimentalSubscribeIncrementally.

Fixes the bug demonstrated in #3709 (which has already been incorporated into the defer-stream branch). This fix is extracted from #3703, which also updates the typing and API around execute. This particular change doesn't affect the API, other than making the `subscribe` return type more honest, as its returned generator could yield AsyncExecutionResult before this change as well. (The reason the previous version built is because every ExecutionResult is actually an AsyncExecutionResult; fixing that fact is part of what #3703 does.)

This PR changes the `execute` and `graphql` APIs on the `defer-stream` branch back to having the same API as on `main`: they can only produce a single `ExecutionResult` and do not support incremental delivery. Incremental delivery is now provided by the new entry point `experimentalExecuteIncrementally`. (We will remove "experimental" once the proposal has been merged into the GraphQL spec.) This function always returns a Promise containing an object that is *either* a single result *or* an initial result plus a generator for subsequent results. This will make upgrading to graphql@17 easier, and increases the clarity of return types. Use distinct types for "the only result of a single-payload execution", "the first payload of a multi-payload execution", and "subsequent payloads of a multi-payload execution", since the data structures are different. (Multi-payload executions *always* have at least one payload, and the structure differs, which is why the new types separate the initial result from subsequent results.) Note that with the previous types, you actually had to use a function with a type guard like `isAsyncIterable`: you couldn't just write `if (Symbol.asyncIterator in result)`. I think both explicitly separating the first (differently-typed) element from the rest of the elements *and* making it possible to differentiate the types with a simple `in` check are improvements. Additionally, somebody using the API in JavaScript who doesn't realize that there is new functionality available who is confused that the result no longer has `data`/`errors` fields can just print out the result and see and search for the terms `initialResult` and `subsequentResults` instead of only seeing iterator internals. Fix `Formatted*Result` types to use `GraphQLFormattedError` rather than `GraphQLError` for `errors` nested inside `incremental`.

robrichard · 2022-08-23T22:47:10Z

src/execution/__tests__/simplePubSub.ts

@@ -51,6 +51,7 @@ export class SimplePubSub<T> {
        emptyQueue();
        return Promise.resolve({ value: undefined, done: true });
      },
+      /* c8 ignore next 4 */


why do we need to ignore these?

Good call. We don't any more! I will check any other additional c8 ignores I added.

robrichard · 2022-08-23T22:48:37Z

src/execution/__tests__/lists-test.ts

@@ -113,7 +113,7 @@ describe('Execute: Accepts async iterables as list value', () => {
        },
      }),
    });
-    return execute({
+    return experimentalExecuteIncrementally({


The tests in this file don't use defer or stream. Why did you change it to use experimentalExecuteIncrementally?

Good catch. It doesn't need to be, and my PR will now remove the | AsyncGenerator from completeObjectList's return type, making it even more clear that the tests in this file don't use defer or stream.

Revert subscribe to v16 API, add new experimentalSubscribeIncrementally

glasser · 2022-08-23T23:27:50Z

Today's updates:

As suggested by @yaacovCR, experimentalExecuteIncrementally now returns PromiseOrValue.
Treat subscribe the same way as I treated execute: subscribe does the same thing as in v6 (with errors in the stream if you use @defer/@stream), and experimentalSubscribeIncrementally supports incremental delivery.
As per spec, initial results may have incremental fields, and all top-level results may have extensions.
Testing improvements from @robrichard.

I encourage squash-and-merge using the current PR description as the commit message.

github-actions · 2022-08-23T23:31:33Z

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

glasser mentioned this pull request Aug 16, 2022

Reference implementation of defer and stream spec #3659

Merged

glasser force-pushed the glasser/defer-stream-simpler-types branch from d1cd417 to be87bf4 Compare August 17, 2022 00:07

robrichard force-pushed the defer-stream branch from 55ac8df to 172c853 Compare August 19, 2022 00:16

glasser force-pushed the glasser/defer-stream-simpler-types branch 2 times, most recently from 4205fd2 to dfc882e Compare August 19, 2022 05:16

glasser force-pushed the glasser/defer-stream-simpler-types branch 2 times, most recently from 752061d to f786f49 Compare August 19, 2022 06:00

glasser marked this pull request as ready for review August 19, 2022 06:02

robrichard force-pushed the defer-stream branch from 172c853 to feb203a Compare August 19, 2022 19:15

glasser mentioned this pull request Aug 19, 2022

defer/stream: fix flattenAsyncIterable concurrency #3710

Merged

glasser force-pushed the glasser/defer-stream-simpler-types branch from f786f49 to 715cc20 Compare August 20, 2022 18:50

glasser changed the title ~~Experiment with simplifying new defer/stream execute types~~ defer/stream: Make new execute result types more precise Aug 20, 2022

glasser force-pushed the glasser/defer-stream-simpler-types branch from 715cc20 to 8e0289c Compare August 23, 2022 02:40

glasser changed the title ~~defer/stream: Make new execute result types more precise~~ defer/stream: split incremental delivery into new entry point Aug 23, 2022

yaacovCR reviewed Aug 23, 2022

View reviewed changes

robrichard reviewed Aug 23, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

glasser commented Aug 23, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

robrichard force-pushed the defer-stream branch from aa830a4 to efda9be Compare August 23, 2022 20:11

glasser added 2 commits August 23, 2022 14:43

add incremental and extensions to all streamed results

7fe995e

glasser force-pushed the glasser/defer-stream-simpler-types branch from 4f4e2a1 to 7fe995e Compare August 23, 2022 21:44

New API returns PromiseOrValue like the old ones

c8d1b85

robrichard reviewed Aug 23, 2022

View reviewed changes

robrichard approved these changes Aug 23, 2022

View reviewed changes

glasser added 2 commits August 23, 2022 16:15

Treat subscribe the same way

fcc378c

Revert subscribe to v16 API, add new experimentalSubscribeIncrementally

Feedback from @robrichard

bee824d

glasser changed the title ~~defer/stream: split incremental delivery into new entry point~~ defer/stream: split incremental delivery into new entry points Aug 23, 2022

This comment has been minimized.

Sign in to view

IvanGoncharov merged commit 5a41f86 into graphql:defer-stream Aug 24, 2022

XiNiHa mentioned this pull request Sep 3, 2022

Let users provide custom executeFn graphql-hive/envelop#1507

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

defer/stream: split incremental delivery into new entry points #3703

defer/stream: split incremental delivery into new entry points #3703

glasser commented Aug 16, 2022 •

edited

Loading

github-actions bot commented Aug 16, 2022

glasser commented Aug 19, 2022 •

edited

Loading

yaacovCR commented Aug 19, 2022

glasser commented Aug 20, 2022

glasser commented Aug 23, 2022

yaacovCR Aug 23, 2022

IvanGoncharov Aug 23, 2022

yaacovCR Aug 23, 2022

IvanGoncharov Aug 23, 2022 •

edited

Loading

IvanGoncharov Aug 23, 2022

robrichard Aug 23, 2022

glasser Aug 23, 2022

IvanGoncharov Aug 23, 2022 •

edited

Loading

This comment has been minimized.

github-actions bot commented Aug 23, 2022

glasser Aug 23, 2022

glasser Aug 23, 2022

This comment has been minimized.

github-actions bot commented Aug 23, 2022

glasser commented Aug 23, 2022

robrichard Aug 23, 2022

glasser Aug 23, 2022

robrichard Aug 23, 2022

glasser Aug 23, 2022

glasser commented Aug 23, 2022 •

edited

Loading

This comment has been minimized.

github-actions bot commented Aug 23, 2022

defer/stream: split incremental delivery into new entry points #3703

defer/stream: split incremental delivery into new entry points #3703

Conversation

glasser commented Aug 16, 2022 • edited Loading

github-actions bot commented Aug 16, 2022

glasser commented Aug 19, 2022 • edited Loading

yaacovCR commented Aug 19, 2022

glasser commented Aug 20, 2022

glasser commented Aug 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanGoncharov Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanGoncharov Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

github-actions bot commented Aug 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

github-actions bot commented Aug 23, 2022

glasser commented Aug 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasser commented Aug 23, 2022 • edited Loading

This comment has been minimized.

github-actions bot commented Aug 23, 2022

glasser commented Aug 16, 2022 •

edited

Loading

glasser commented Aug 19, 2022 •

edited

Loading

IvanGoncharov Aug 23, 2022 •

edited

Loading

IvanGoncharov Aug 23, 2022 •

edited

Loading

glasser commented Aug 23, 2022 •

edited

Loading