wip @defer support #1077
wip @defer support #1077
Conversation
v2/pkg/engine/resolve/resolve.go
Outdated
| defer close(ch) | ||
| go func() { | ||
| buf := &bytes.Buffer{} | ||
| if _, err := r.ResolveGraphQLResponse(ctx, response.DeferredResponse, nil, buf); err != nil { |
There was a problem hiding this comment.
This assumes that there's just one single deferred Response.
v2/pkg/engine/resolve/resolve.go
Outdated
| executor chan func() | ||
| } | ||
|
|
||
| func (r *Resolver) ResolveGraphQLIncrementalResponse(ctx *Context, response *GraphQLIncrementalResponse, writer io.Writer) error { |
There was a problem hiding this comment.
I'd create an interface as writer, e.g. IncrementalResponseWriter with 3 funcs, write, flush, complete. This way, you don't have to think about boundaries or transports here. Just write a JSON and flush it. Let the implementer of the writer adapt the transport, e.g. by adding a boundary after flush.
v2/pkg/engine/plan/visitor.go
Outdated
| } | ||
|
|
||
| if streaming && operationKind != ast.OperationTypeQuery { | ||
| v.Walker.StopWithExternalErr(operationreport.ErrDeferStreamNotAllowedOutsideQuery()) |
There was a problem hiding this comment.
What's wrong with having defer on Mutations?
v2/pkg/engine/plan/visitor.go
Outdated
|
|
||
| func (v *Visitor) LeaveFragmentSpread(ref int) { | ||
| v.debugOnLeaveNode(ast.NodeKindFragmentSpread, ref) | ||
| v.inDeferredFragment = false |
There was a problem hiding this comment.
If you walk into a deferred fragment spread, then inside another fragment spread, then out of the nested spread, you're still in the parent spread but your logic now says we're out because we left the nested one.
One solution we use for such a problem is to create a map of fragment spread ref's that are deferred.
When you enter, you add it, when you leave, you verify if you left a deferred one or not.
The problem now is that when you leave a fragment spread, you need to know if this was already deferred by a parent. So this map could contain the ref of the fragment spread as the key, and a struct with information if we're inside a deferred parent. Then we can properly set if we're within a deferred fragment.
There's just a question of how valuable is the information that we're inside a deferred fragment when they can be nested. Maybe we need to also know in which deferred fragment we are, but I haven't digged enough into the code.
There was a problem hiding this comment.
Yeah, at this point I'm reworking this as a stack. It doesn't look like we need anything that isn't available when we leave, but I haven't addressed label or if yet.
There was a problem hiding this comment.
You don't have to worry about "if". We can do the same thing like we're doing with include and skip, we "normalize" defer out when if evaluated to false.
v2/pkg/engine/resolve/resolve.go
Outdated
| for _, deferred := range response.DeferredResponses { | ||
| g.Go(func() error { | ||
| buf := &bytes.Buffer{} | ||
| if _, err := r.ResolveGraphQLResponse(ctx, deferred.ImmediateResponse, nil, buf); err != nil { |
There was a problem hiding this comment.
I'd rewrite this. If you look a bit into the code, you'll see that we create a "tools" struct, which contains a "resolvable" which contains "data". We can keep this tools throughout all steps and do the following flow:
- resolve the initial data
- print and flush the initial response
- iterate over the incremental response parts
- for each part, use the same tools to "add" more data to it
- once the data was loaded with the Loader, we render it into the writer and flush
- repeat for all incremental parts until all data is loaded and resolved
The "missing" link is that we need to split the "resolve" part into incremental resolve objects, always keeping a reference path to where in the "data" object this incremental part should render from.
In addition, each incremental part needs to contain information on how to render the incremental meta information.
still need to get more complex test cases passing
carldunham
force-pushed
the
cd/defer-stream
branch
from
eca757b to
7d42e76
Compare
|
Closing in favor of #1108 |
2 participants
Pretty rough. Still need to clean up tests and do real-world testing.
@streamshould be an easy add-on. Will need to consider things like holding the concurrency token, etc.