Support for experimental @defer & @stream #583

robrichard · 2020-01-09T00:09:47Z

Support for streaming multi-part http responses as implemented by

graphql/graphql-js#2319
https://github.com/relay-tools/fetch-multipart-graphql

acao · 2020-02-22T21:26:59Z

looks great, thanks @robrichard! once we get one of the graphql-js proposals merged for 15.0.0 we can revisit the tests here. saw there are two proposal PRs for graphql-js

danielrearden · 2020-10-28T13:56:46Z

src/index.ts

+    if (isAsyncIterable(executeResult)) {
+      response.setHeader('Content-Type', 'multipart/mixed; boundary="-"');
+      sendPartialResponse(pretty, response, formattedResult);
+      for await (let payload of executeResult) {


@robrichard What happens when the request is terminated by the client (i.e. I navigate away from the page or close the browser)? Would it make sense to add an event listener to response for the close event and call the AsyncIterator's return method inside?

Yes, I think that makes sense. I will implement that.

I added a handler for the close event to call return on the underlying async iterable.

@IvanGoncharov this required disabling some lint rules (see d883ae7#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80R419). Do you think this is ok or do you have a better way to implement this?

n1ru4l · 2020-10-28T16:04:34Z

I got a bit too excited so I created a playground making it easier for people to test this new stuff! https://github.com/n1ru4l/graphql-bleeding-edge-playground

Awesome work!!!

acao · 2020-10-28T16:08:35Z

@n1ru4l awesome! it looks great in graphiql as well. we are going to archive playground soon though :(

n1ru4l · 2020-10-28T16:10:06Z

ah wait it is graphiql 😂 playground ist just the name of the repo it uses graphiql :D

Please check out the GraphiQL fetchers 😇

acao · 2020-10-28T16:11:09Z

@n1ru4l oh awesome I see, thanks for that! did you see this? graphql/graphiql#1700 (comment)

acao · 2020-10-28T16:18:57Z

@n1ru4l are you interested in helping me create this PR in graphiql monorepo for the new fetcher patterns? Otherwise I might not have time until next weekend :/

n1ru4l · 2020-10-29T09:54:42Z

@robrichard Would it be possible for you to do single commits instead of force-pushing, I am having a hard time following the changes 😅 I also updated the playground to the latest commit.

robrichard · 2020-10-29T15:34:04Z

I've been force pushing because it's harder to keep rebasing when there are a lot of commits and this PR has been open for a long time.

n1ru4l · 2020-10-30T14:02:47Z

src/index.ts


-      // Collect and apply any metadata extensions if a function was provided.
-      // https://graphql.github.io/graphql-spec/#sec-Response-Format
+      if (isAsyncIterable(executeResult)) {


Since executeFn is typed to return an AsyncIterableIterator, it would make sense to instead use a isAsyncIterableIterator function. That way it is also possible to call executeResult.return?.() in case the request is aborted without adding ts-ignores.

Nevermind, if you use the isAsyncIterable function the type checker already is aware of the actual type (AsyncIterableIterator), making it easy to call .return()

n1ru4l · 2020-10-30T14:05:33Z

It would also make sense to adjust the renderGraphiQL function to provide a multipart compatible fetcher

IvanGoncharov · 2020-11-11T16:36:30Z

src/index.ts

+        // Get first payload from AsyncIterator. http status will reflect status
+        // of this payload.
+        const asyncIterator = getAsyncIterator<ExecutionResult>(executeResult);
+        const { value } = await asyncIterator.next();


await can throw and should be inside try 🔼

test for that would be great. You can do that by using custom executeFn that return an async iterator.

@IvanGoncharov do you think it makes sense for the try/catch that is currently wrapping await executeFn({...}) to be expanded to also wrap this?

@robrichard Yes, I think it would work 👍

maraisr · 2020-11-14T09:02:50Z

src/index.ts

+  result: FormattedExecutionResult | FormattedExecutionPatchResult,
+): void {
+  const json = JSON.stringify(result, null, pretty ? 2 : 0);
+  const chunk = Buffer.from(json, 'utf8');


will this always be utf8?

this has me wanting to bring up protocolbuffers, messagepack and other binary formats, as well as yaml, however that seems way out of scope for this PR, and should probably begin as an HTTP transport RFC proposal. GraphiQL users have on multiple occasions requested support for these formats, especially for server-to-server transmissions and iot

back to this discussion, though, would it work to use a binary encoding format latin1 with JSON? I've never tried that myself. if someone was returning a binary format like BSON for example, then we should be returning content type of application/bson, right? Is there a case where someone would be sending application/json and not using utf8 encoding?

if there are indeed more ways to mix and match charset and content type with JSON than I'm aware of, maybe this should be controlled by the request headers then, and fall back to utf8 as default?

Worth noting that the transport RFC spec doesn't require utf8 necessarily (yet):
https://github.com/graphql/graphql-over-http/blob/master/rfcs/IncrementalDelivery.md

and the reference client implementation expects utf8:
https://github.com/relay-tools/fetch-multipart-graphql/blob/dfa54d0c649ecbbedac3e9764998f63894367505/src/fetch.js#L26

meros also at this time uses UTF8. It's a force of habit. But there might be people out there needing some out of the ordinary encodings.

Maybe it's a cross that bridge when we get there?

express-graphql already only sends utf8 so I don't think there's any change in behavior here: https://github.com/graphql/express-graphql/blob/master/src/index.ts#L531

As far as the spec, I'm not sure if it's worth specifying what encodings are required. FWIW it seems like JSON requires either UTF-8, UTF-16, or UTF-32 but recommends UTF8 for maximum interoperablity.

https://tools.ietf.org/html/rfc7159#section-8.1

maraisr · 2020-11-14T09:03:41Z

Wish github had stacked PR's 😔 robrichard#1

sibelius approved these changes Jan 9, 2020

View reviewed changes

acao added the PR: feature 🚀 requires increase of "minor" version number label Feb 22, 2020

robrichard force-pushed the defer-stream branch 2 times, most recently from 2492b68 to 8dc8565 Compare July 3, 2020 00:12

danielrearden mentioned this pull request Jul 7, 2020

Middleware refactor #675

Open

robrichard force-pushed the defer-stream branch from 46b3ff3 to 3505ce2 Compare October 15, 2020 15:33

acao mentioned this pull request Oct 25, 2020

add support for defer and stream directives graphql/graphql-js#2319

Merged

danielrearden reviewed Oct 28, 2020

View reviewed changes

robrichard force-pushed the defer-stream branch 3 times, most recently from 7c2e919 to 06f9ded Compare October 28, 2020 15:21

n1ru4l reviewed Oct 30, 2020

View reviewed changes

IvanGoncharov reviewed Nov 11, 2020

View reviewed changes

robrichard force-pushed the defer-stream branch 3 times, most recently from df074c9 to aa62e24 Compare November 11, 2020 22:45

maraisr reviewed Nov 14, 2020

View reviewed changes

robrichard mentioned this pull request Nov 16, 2020

Placeholder issue for feedback on @defer & @stream proposal graphql/graphql-js#2848

Closed

robrichard force-pushed the defer-stream branch from b060480 to d883ae7 Compare November 17, 2020 21:07

support for experimental defer-stream

378f44b

catch errors from AsyncIterable

928b129

robrichard force-pushed the defer-stream branch 3 times, most recently from 78fdee3 to e303a2c Compare November 19, 2020 15:41

call return on underlying async iterator when connection closes

ce8429e

robrichard force-pushed the defer-stream branch from e303a2c to ce8429e Compare November 19, 2020 15:43

IvanGoncharov changed the base branch from master to defer-stream November 20, 2020 12:12

IvanGoncharov merged commit 95e7324 into graphql:defer-stream Nov 20, 2020

robrichard mentioned this pull request Nov 23, 2020

Add support for defer and stream directives (feedback is welcome) #726

Open

robrichard added a commit that referenced this pull request Dec 3, 2020

Support for experimental @defer & @stream (#583)

1fa2eb1

robrichard added a commit that referenced this pull request Feb 9, 2021

Support for experimental @defer & @stream (#583)

10f6843

Support for experimental @defer & @stream #583

Support for experimental @defer & @stream #583

Uh oh!

Conversation

robrichard commented Jan 9, 2020

Uh oh!

acao commented Feb 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n1ru4l commented Oct 28, 2020

Uh oh!

acao commented Oct 28, 2020

Uh oh!

n1ru4l commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acao commented Oct 28, 2020

Uh oh!

acao commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n1ru4l commented Oct 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robrichard commented Oct 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n1ru4l Nov 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n1ru4l commented Oct 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acao Nov 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maraisr Nov 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maraisr commented Nov 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

n1ru4l commented Oct 28, 2020 •

edited

Loading

acao commented Oct 28, 2020 •

edited

Loading

n1ru4l commented Oct 29, 2020 •

edited

Loading

n1ru4l Nov 1, 2020 •

edited

Loading

acao Nov 14, 2020 •

edited

Loading

maraisr Nov 14, 2020 •

edited

Loading