Support returning async iterables from resolver functions #2712

robrichard · 2020-07-15T14:17:33Z

Returning AsyncIterables in resolver methods is useful when @stream is supported and can be added without any breaking changes. This is split out from #2319

robrichard · 2020-07-15T14:51:36Z

src/subscription/subscribe.js

@@ -160,7 +159,7 @@ function subscribeImpl(
    // Note: Flow can't refine isAsyncIterable, so explicit casts are used.
    isAsyncIterable(resultOrStream)
      ? mapAsyncIterator(
-          ((resultOrStream: any): AsyncIterable<mixed>),
+          resultOrStream,


@IvanGoncharov since adding boolean %checks(value instanceof AsyncIterable) to isAsyncIterable I was able to remove the cast here, but I was not able to remove it from the other side of the ternary.

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/subscription/subscribe.js:166:9 Cannot call sourcePromise.then with function bound to onFulfill because mixed [1] is incompatible with object type [2] in type argument Yield [3] of the return value of property @@asyncIterator of the return value. [incompatible-call] src/subscription/subscribe.js [2] 117│ ): Promise<AsyncIterator<ExecutionResult> | ExecutionResult> { : 163│ mapSourceToResponse, 164│ reportGraphQLError, 165│ ) 166│ : resultOrStream, 167│ ); 168│ } 169│ : [1] 206│ ): Promise<AsyncIterable<mixed> | ExecutionResult> { /private/tmp/flow/flowlib_5bfb8b1/core.js [3] 562│ interface $AsyncIterator<+Yield,+Return,-Next> {

IvanGoncharov · 2020-07-15T18:30:26Z

src/__tests__/starWarsSchema.js

+        let i = 0;
+        for (const friend of friends) {
+          // eslint-disable-next-line no-await-in-loop
+          await new Promise((r) => setTimeout(r, 1));


Why do you need setTimeout?
A promise is always processed on a next tick even if it already resolved.

Leftover from when I was trying to time results for tests combining @stream and @defer. Not needed any more and removed from this PR

IvanGoncharov · 2020-07-15T18:31:36Z

src/__tests__/starWarsSchema.js

+          i++;
+        }
+        // close iterator asynchronously
+        await new Promise((r) => setTimeout(r, 1));


IvanGoncharov · 2020-07-15T19:46:35Z

src/__tests__/starWarsSchema.js

+      description:
+        'The friends of the droid, or an empty list if they have none. Returns an AsyncIterable',
+      args: {
+        errorIndex: { type: GraphQLInt },


It's better to move non-trivial tests here: https://github.com/graphql/graphql-js/blob/master/src/execution/__tests__/lists-test.js

Starwars schema is intended as just a sample schema to quickly demonstrate all basic features.

Removed these tests and added new tests to lists-test.js

IvanGoncharov · 2020-07-15T19:48:45Z

src/execution/execute.js

+ * Complete a async iterable value by completing each item in the list with
+ * the inner type
+ */
+


IvanGoncharov · 2020-07-15T20:12:37Z

src/execution/execute.js

+    return iterator.next().then(
+      ({ value, done }) => {
+        if (done) {
+          return;


You can return completedResults here right?
And save one tick.

Yes, updated to return completedResults here and in the error handler.

IvanGoncharov · 2020-07-15T20:13:56Z

src/execution/execute.js

+
+  const itemType = returnType.ofType;
+
+  const handleNext = () => {


Usually, if we don't need this we use standard function syntax and put such functions after return of the main function.

I refactored this a little bit so the handleNext function is no longer required. We do not need to create inline functions which rely on closures any more.

IvanGoncharov · 2020-07-15T20:16:55Z

src/execution/execute.js

+  const itemType = returnType.ofType;
+
+  const handleNext = () => {
+    const fieldPath = addPath(path, index);


Can you please check if you need to add third argument.

I'm not sure I understand what the third argument is for and when it should be passed. I updated it to pass undefined as that's what's done currently for list values:

graphql-js/src/execution/execute.js

Line 944 in 6012d28

const fieldPath = addPath(path, index, undefined);

robrichard · 2020-07-30T12:27:27Z

@IvanGoncharov this is ready for another review whenever you have some time.

IvanGoncharov · 2020-07-31T17:44:46Z

@robrichard Had a surgery so was offline for the last few weeks.
Now returning back to open-source so it will take me a few days to get back to this PR.

robrichard · 2020-08-01T13:08:20Z

@IvanGoncharov no rush, please take your time

IvanGoncharov · 2020-08-06T16:17:10Z

@robrichard I merged some changes to subscribe can you please rebase it?
It should pretty simple.

robrichard · 2020-08-06T16:47:15Z

@IvanGoncharov this is up to date now

IvanGoncharov · 2020-08-07T08:20:27Z

src/execution/execute.js

+    const iteratorMethod = result[SYMBOL_ASYNC_ITERATOR];
+    const iterator = iteratorMethod.call(result);


@robrichard Can we do?

Suggested change

const iteratorMethod = result[SYMBOL_ASYNC_ITERATOR];

const iterator = iteratorMethod.call(result);

const iterator = result[SYMBOL_ASYNC_ITERATOR](result);

Yes, updated.

IvanGoncharov · 2020-08-20T17:48:37Z

@robrichard Thanks for your patience and sorry for such long delay.
I reviewed this PR in more detail and suspect we not handling a few cases like: completeValue throwing or returning a promise, etc.
Also, I feel like we need to figure out a way to unify more code between iterating async iterable and other types of collections.
In the next few days, I will try to add as much as possible missing test cases and we can proceed from there.

IvanGoncharov · 2020-08-25T18:03:06Z

@robrichard I rebased this PR against master and added trivial commit on top of it.
To allow both of us to submit and review each other PRs I created branch asynciteratable and merge it there.

Co-authored-by: Ivan Goncharov <[email protected]>

Co-authored-by: Ivan Goncharov <[email protected]> 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

robrichard force-pushed the asynciterable branch from 57ca245 to 08acc44 Compare July 15, 2020 14:36

robrichard mentioned this pull request Jul 15, 2020

add support for defer and stream directives #2319

Merged

robrichard force-pushed the asynciterable branch from 08acc44 to 39a4c55 Compare July 15, 2020 14:48

robrichard commented Jul 15, 2020

View reviewed changes

IvanGoncharov reviewed Jul 15, 2020

View reviewed changes

IvanGoncharov added the PR: feature 🚀 requires increase of "minor" version number label Jul 15, 2020

IvanGoncharov reviewed Jul 15, 2020

View reviewed changes

robrichard force-pushed the asynciterable branch from bc704f9 to 56d2ba0 Compare July 27, 2020 14:16

robrichard force-pushed the asynciterable branch from 56d2ba0 to 1ff8a36 Compare August 6, 2020 16:43

IvanGoncharov reviewed Aug 7, 2020

View reviewed changes

robrichard force-pushed the asynciterable branch 7 times, most recently from 93bbece to 0ef8163 Compare August 12, 2020 21:59

robrichard force-pushed the asynciterable branch from 0ef8163 to 65450d3 Compare August 19, 2020 12:15

IvanGoncharov added this to the 15.x.x milestone Aug 20, 2020

robrichard and others added 2 commits August 25, 2020 19:45

Support returning async iterables from resolver functions

42c83f3

Move AsyncIteratable tests in to separate block

f0c3215

IvanGoncharov force-pushed the asynciterable branch from 65450d3 to f0c3215 Compare August 25, 2020 17:52

IvanGoncharov changed the base branch from master to asynciteratable August 25, 2020 17:58

IvanGoncharov merged commit f612eb6 into graphql:asynciteratable Aug 25, 2020

IvanGoncharov mentioned this pull request Aug 25, 2020

Support returning async iterables from resolver functions #2757

Merged

robrichard deleted the asynciterable branch October 4, 2020 14:55

robrichard added a commit to robrichard/graphql-js that referenced this pull request Oct 4, 2020

Support returning async iterables from resolver functions (graphql#2712)

0432956

Co-authored-by: Ivan Goncharov <[email protected]>

robrichard added a commit to robrichard/graphql-js that referenced this pull request Oct 4, 2020

Support returning async iterables from resolver functions (graphql#2712)

6dcba93

Co-authored-by: Ivan Goncharov <[email protected]>

robrichard added a commit to robrichard/graphql-js that referenced this pull request Oct 9, 2020

Support returning async iterables from resolver functions (graphql#2712)

d439bd5

Co-authored-by: Ivan Goncharov <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support returning async iterables from resolver functions #2712

Support returning async iterables from resolver functions #2712

robrichard commented Jul 15, 2020

robrichard Jul 15, 2020

IvanGoncharov Jul 15, 2020

robrichard Jul 15, 2020

IvanGoncharov Jul 15, 2020

robrichard Jul 15, 2020

IvanGoncharov Jul 15, 2020

robrichard Jul 15, 2020

IvanGoncharov Jul 15, 2020

IvanGoncharov Jul 15, 2020

robrichard Jul 15, 2020

IvanGoncharov Jul 15, 2020 •

edited

Loading

robrichard Jul 15, 2020 •

edited

Loading

IvanGoncharov Jul 15, 2020

robrichard Jul 15, 2020

robrichard commented Jul 30, 2020

IvanGoncharov commented Jul 31, 2020

robrichard commented Aug 1, 2020

IvanGoncharov commented Aug 6, 2020

robrichard commented Aug 6, 2020

IvanGoncharov Aug 7, 2020

robrichard Aug 7, 2020

IvanGoncharov commented Aug 20, 2020

IvanGoncharov commented Aug 25, 2020


		const itemType = returnType.ofType;

		const handleNext = () => {

		const iteratorMethod = result[SYMBOL_ASYNC_ITERATOR];
		const iterator = iteratorMethod.call(result);

	const iteratorMethod = result[SYMBOL_ASYNC_ITERATOR];
	const iterator = iteratorMethod.call(result);
	const iterator = result[SYMBOL_ASYNC_ITERATOR](result);

Support returning async iterables from resolver functions #2712

Support returning async iterables from resolver functions #2712

Conversation

robrichard commented Jul 15, 2020

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanGoncharov Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

robrichard Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robrichard commented Jul 30, 2020

IvanGoncharov commented Jul 31, 2020

robrichard commented Aug 1, 2020

IvanGoncharov commented Aug 6, 2020

robrichard commented Aug 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanGoncharov commented Aug 20, 2020

IvanGoncharov commented Aug 25, 2020

IvanGoncharov Jul 15, 2020 •

edited

Loading

robrichard Jul 15, 2020 •

edited

Loading