Placeholder issue for feedback on `@defer` & `@stream` proposal

[robrichard]

For each release of GraphQL.js, we are now publishing an accompanying release containing an experimental implementation of the @defer and @stream directives. We are hoping to get community feedback on these releases before the proposal is accepted into the GraphQL specification.

You can use this experimental release of GraphQL.js by adding the following to your project's package.json file.

"graphql": "experimental-stream-defer"

A similar release of express-graphql will be coming soon.

I am creating this issue as a placeholder for anyone who has tried out these experimental packages to leave feedback. Please let us know if everything is working as expected or if you run into any issues. Any suggestions for improvements are also welcome.

References:

• Demo repo
• Conference talk about defer/stream
• defer/stream RFC
• Spec edits
• graphql-js branch
• express-graphql PR
• graphql-over-http RFC
• fetch-multipart-graphql (client library)
• meros (client library)

Feedback received so far

Call return on iterable when connection closes

• Raised by @danielrearden
• graphql-js
  • Fixed: Return underlying AsyncIterators when execute result is returned #2843
  • Released: TBD
• express-graphql
  • Fixed: graphql/express-graphql@ce8429e
  • Released: express-graphql@0.12.0-experimental-stream-defer.1

content-length in payload should not be required, parsers should use boundary instead

• Raised by @maraisr
• graphql-over-http spec
  • Fixed: Spec edits for multipart @defer @stream around content-length graphql-over-http#152
• fetch-multipart-graphql
  • Fix: WIP
• express-graphql
  • Fix: feat: remove the need for content-length robrichard/express-graphql#1

[danielrearden]

@IvanGoncharov It would be helpful if we could cut a new experimental-stream-defer release with the latest changes to the branch (i.e. 163f62a).

[IvanGoncharov]

@danielrearden Planning to have bug fix release for master today/tomorrow so we can synchronize both.

[tgriesser]

@danielrearden pointed out this thread after I added feedback on the PR.

My question was around whether the graphql / execute functions should be overloaded with the new return type, or if it'd be simpler and maybe more backward compatible to have a new entrypoint graphqlStream supporting the new behavior:

graphql(): Promise<ExecutionResult>
graphqlSync(): ExecutionResult
graphqlStream(): AsyncIterableIterator<ExecutionResult>

Commenting on @danielrearden's followup:

@tgriesser That would require the server implementation to walk the entire operation AST to determine if any @stream or @defer directives were used in the first place, though, right?

I don't think it'd necessarily need to be an ahead of time validation (though that could be a nice feature), I was thinking it'd be similar to executeSync / graphqlSync, where it's just a simple runtime validation after the operation is executed:

graphql-js/src/graphql.js

Lines 155 to 161 in cd273ad

	// Assert that the execution was synchronous.
	if (isPromise(result)) {
	throw new Error('GraphQL execution failed to complete synchronously.');
	}
	return result;
	}

Except in this case it'd be adding the check to the execute / graphql:

export function execute(args: ExecutionArgs): PromiseOrValue<ExecutionResult> {
  const result = executeImpl(args);

  // Assert that the execution wasn't an async iterable.
  if (isAsyncIterable(result)) {
    result.return()
    throw new Error('Streaming GraphQL must be called with executeStream.');
  }

  return result;
}

Mostly came up thinking of situations where I've used the graphql or execute directly outside of a server implementation, and now needing to guard the result from a type standpoint.

[robrichard]

@tgriesser we did discuss a few similar ideas, and at one point we did implement separate experimental graphql/execute functions, but this was scrapped because it required too much code duplication.

I think long term, most server implementations would want to support AsyncIterable results. Adding a separate function with the current return type (Promise<ExecutionResult | AsyncIterable<AsyncExecutionResult>>) would only be beneficial in the short term to reduce breaking changes.

A new function that always returns an AsyncIterable (with just one result when defer/stream is not used) is an interesting idea that was brought up at one of the working group meetings. I'm not opposed to this approach if this is the direction the library maintainers want to take. I think it does adds some overhead as servers will either have to hold on to the first response to see if the iterable closes or send every response as a chunk encoded connection

cc @IvanGoncharov

[jensneuse]

I've implemented @defer & @stream a while ago and wanted to share my experience.
I fully agree on the approach to apply @defer on inline fragments & @stream on list fields.

list field directive location

I think it would be beneficial if we add a new directive location "LIST_FIELD" to the spec.
This enables validation for IDE's like GraphiQL to make the user alert when they apply the @stream directive on non list fields.

content negotiation

First I think it would make sense to add custom vendor headers for both client and server to indicate if they support defer & stream.

Possible implementations:

The client indicates it is capable of version 2.

Accept: application/vnd.graphql.org+json;version=2

The client supports streaming responses.

Accept: application/json;graphql/streaming

Server response header:

Content-Type: application/json;graphql/streaming

Why do I think this is important?

GraphQL Operations might be re-used by different clients or stored. Therefore it's not always the case that the client who initially wrote the Operation will also send it. By using content negotiation clients and servers can decide if they "upgrade" the request to be a defer/stream request or not.

Another use case is proxies & API gateways. It might not be desirable for an intermediate to ask for a deferred response. With content negotiation, it could still use the same Operation but is capable to disable defer+stream via a header.

If the client & server don't use the headers specified we can assume they are pre defer/stream and disable it by default, making it backwards compatible.

response format

The current RFC proposes a custom format with "path", "data" and "hasNext".
I'd like to propose a simpler model which I think is easier to adopt for client and server implementers.

The first response should be an ordinary GraphQL response object.
The Content-Type header indicates that the response will be streamed.
We don't have to distinguish between @stream and @defer as both are streaming responses.

Subsequent streaming responses will be JSON Patch objects, delimited with two newline characters.
JSON Patch is a standard to update an existing JSON object with one or many Patches.
JSON Patch has implementations in many languages, making adoption for clients & servers easier.

The final response will be followed by three newline characters, indicating the end of the stream.

Using newline characters instead of "hasNext" makes it easy for clients to understand the state of the stream without looking into the payload directly.

[jensneuse]

@jensneuse thank you so much for the feedback. Just ok the point of accept headers, I think that can be done currently but just saying that we accept "multipart/mixed". Instead of a bespoke graphql vendor mime. What do you think?

Tbh I'm not yet sure what is the best way to represent this.
I just have a feeling that multipart/mixed is too high level and too vague for a client and server to negotiate the protocol they want to use for communication. Why not make it more explicit?

[AlicanC]

Subsequent streaming responses will be JSON Patch objects

Would we ever need any other JSON Patch operation than "add"?

If not, we could just:

• Rename "data" to "value"
• Make "path" a JSON Pointer

Then it would be really easy to generate a JSON Patch from the GQL format:

const oldGqlFormat = {
  data: data,
  path: gqlPath,
  label,
  hasNext,
  errors,
  extensions,
};

const newGqlFormat = {
  value: data,
  path: jsonPointer,
  label,
  hasNext,
  errors,
  extensions,
};

const jsonPatch = [
  {
    op: 'add',
    // We would also be spreading a bunch of other stuff here,
    // but JSON Patch spec says they will be ignored so we should be ok
    ...newGqlFormat,
  },
];

Going full JSON Patch also sounds interesting. Maybe we could also use the "replace" operation for updating existing data in some other proposal.

[jensneuse]

@AlicanC I'm already doing exactly what you're proposing.
In one case I initially send a field with the value "null".
Then I stream JSON Patches, one of them contains a "replace" directive, pointing to the "null" value.
This makes the client extremely simple and "dumb" because it doesn't have to know anything about the content.
It simply keeps the last version of the JSON response and updates it until the server terminates the connection.

This also works well for adding Errors while resolving deferred/streamed responses.
The server simply has to emit an "add" Operation with an Error Object as value and the path leading to the errors array.

Here's a snippet how a client looks like:

let hasMessages = false;
let lastMessage:any;
source = new EventSource("example.com/graphql");
source.onmessage = e => {
    const nextMessage = JSON.parse(e.data);
    if (lastMessage !== undefined && nextMessage instanceof Array){
        lastMessage = applyPatch(lastMessage, nextMessage).newDocument;
        opts.next(lastMessage);
    } else {
        opts.next(nextMessage);
        lastMessage = nextMessage;
    }
    hasMessages = true;
}

It's based on EventSource is just an implementation detail.
Could be just an ordinary HTTP/2 Stream, multipart, chunked encoding, anything.

Checking if nextMessage is an Array, in this case, is sufficient because JSON Patch Objects are always Arrays whereas the initial GraphQL Response is a JSON Object.

"applyPatch" is from the npm package "fast-json-patch".

I didn't intentionally choose this approach.
It was more me being lazy and I didn't want to implement updating JSON objects myself.
So maybe we can improve this approach or figure out why it's bad.
I just wanted to share my experience here and see if it can help find a good solution.

[jensneuse]

There's one more topic I'd like to talk about and that's about client performance.
While implementing defer, I learned that it's a problem for Javascript clients when you update too often with small chunks.

So while it looks like a working solution what you propose, it doesn't scale under high load.
To test this I've created a high-performance service, written in Golang, to stream updates extremely fast.
By that, I mean thousands of Patches per second.

What happens is that the Javascript client gets completely overwhelmed by the number of Patches to apply.
It's independent if you're using JSON Patch or a custom merge algorithm like you proposed.

The solution was rather simple, I've introduced batching with a default flush interval.
This means, during a window of e.g. 10ms, I collect all JSON Patch Objects into an Array.
Once we hit a maximum amount of Patches (e.g. 100) OR the window limit (e.g. 10ms) we Flush the buffer.
Once the buffer is flushed, we continue resolving until the complete tree is resolved.

If you want to get a bit more context, I've written about it here:
https://wundergraph.com/features/federation

I believe that if you don't address this issue you'll run into problems once you scale streams up.
Luckily, JSON Patch already allows us to batch updates using an Array of Patch Objects and that's why I propose this as a possible solution.

[robrichard]

@jensneuse thanks for the feedback! Here's our thoughts on these topics

content-negotiation

GraphQL Operations might be re-used by different clients or stored. Therefore it's not always the case that the client who initially wrote the Operation will also send it.

In this case could the clients make use of the if argument on @defer and @stream? The clients that do not support incremental delivery can pass a false variable.

query SharedQuery($shouldStream: Boolean) {
  myListField(if: $shouldStream, initialCount: 0) {
    name
  }
}

I think this would solve this issue without relying on out-of-band information being passed?

Response format

hasNext

I think there is benefit from hasNext being part of the specified payload instead of deriving it from the underlying transport mechanism. These requests may be used over other transports where inspecting boundaries is not as easy and we wouldn’t want to tie the GraphQL spec to closely to one transport.

Since many GraphQL clients are transport agnostic I think it’s convenient that there is a standard way to determine if a request is still in flight or not, while keeping the transport code fully encapsulated.

JSON Patch

As @AlicanC said, the only JSON patch operation that’s needed is add, but for @defer it is really an object spread. You would need a JSON patch add operation for each field in the deferred fragment.

For example this payload:

{
  "data": {
    "firstName": "Rob",
    "lastName": "Richard",
    "username": "robrichard",
  },
  "path": ["user"],
  "hasNext": false
}

would be equivalent to this JSON patch:

[
  { "op": "add", "path": "/person/firstName", "value": "Rob"},
  { "op": "add", "path": "/person/lastName", "value": "Richard"},
  { "op": "add", "path": "/person/username", "value": "robrichard"},
]

Plus we need the additional fields for label, extensions, and errors.

While json-patch libraries make it easy to apply patches to the response, many GraphQL clients like Relay and Apollo use a normalized store for data. So incoming patch results would not actually be applied to the previous payload, but rather normalized and inserted into the store.

The GraphQL spec already has a definition for referencing response paths, currently used for errors. We are reusing this definition for incremental delivery, so I’d be hesitant to also introduce JSON Pointers into the spec.

Client Performance

Agreed that a flood of payloads can easily overwhelm clients. I think it could be handled client side with debouncing if a large response is potentially expected.

For example - Relay supports returning an array of payloads from the network layer. It will process all of the payloads into the store and only trigger one re-render of the React components.