Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Combine subscribe with execute? #2144

Open
ericls opened this issue Aug 31, 2019 · 7 comments
Open

Combine subscribe with execute? #2144

ericls opened this issue Aug 31, 2019 · 7 comments

Comments

@ericls
Copy link

ericls commented Aug 31, 2019

Right now, subscribe and execute are two functions. And there are some issues with that.

  1. Semantically, subscription is a type of execution. In the spec, Query, Mutation and Subscription are on the same level. Subscription is not an outlier.
  2. When I get a Document, I need to go through validations to get the Operation, and then call execute or subscribe based on Operation's type, and they go through a similar validation, resulting in duplicated work.
@ericls ericls changed the title Combine subscription with execute? Combine subscribe with execute? Aug 31, 2019
@IvanGoncharov
Copy link
Member

@ericls Main difference is in the return value since subscribe returns promise to async iterator:

): Promise<AsyncIterator<ExecutionResult> | ExecutionResult>;

Another big difference is that execute can return the result without wrapping it in a promise:

): PromiseOrValue<ExecutionResult>;

and then call execute or subscribe based on Operation's type, and they go through a similar validation,

Both execute and subscribe doesn't do the validation.

resulting in duplicated work.

Since both functions accept args as objects you can share most of the code:

const args = { schema, document, /* ... */ };
switch(getOperationRootType(document, operationName).operation) {
  case 'query':
  case 'mutation':
    return execute(args);
  case 'subscribe':
    const asyncIter = await subscribe(args);
    // send event's using asyncIter
}

@ericls
Copy link
Author

ericls commented Sep 1, 2019

@IvanGoncharov, Thank you for you reply!

But both execute and subscribe calls buildExecutionContext, which validates and selects an operation from Document. And the signature of getOperationRootType is (s: Schema, o: Operation) which requires operation to be known.

@IvanGoncharov
Copy link
Member

And the signature of getOperationRootType is (s: Schema, o: Operation) which requires operation to be known.

@ericls Sorry for the confusion I confused it with:

export function getOperationAST(
documentAST: DocumentNode,
operationName: ?string,
): ?OperationDefinitionNode {

But both execute and subscribe calls buildExecutionContext, which validates and selects an operation from Document.

It's pretty basic validation (I thought you mean full query validation made by validate) and as you correctly pointed out code is shared code is already extracted into buildExecutionContext.


I agree that in theory, it would be great to unify the execution of query, mutation and subscription under a single function. In practice, it would be a breaking change
and result in function that returns ExecutionResult | Promise<AsyncIterator<ExecutionResult> | ExecutionResult> which is not the best DX.

So it's something that we case explore post 15.x.x and see if someone can come up good API for such function and a plan on how to minimize breaking change.

@ericls
Copy link
Author

ericls commented Sep 3, 2019

@IvanGoncharov , thanks again for you reply.

buildExecutionContext is indeed very helpful. I think I'll use it for now.

@jasonkuhrt
Copy link

jasonkuhrt commented Jul 6, 2020

Just happened this use-case. As a user, my initial expectation was that I would be able to do query.execute. I was initially surprised to learn that subscription was a separate method.

But I sympathize with how the polymorphic type this would result in could hurt DX.

A new TS generic parameter and/or casting could do their bit to help, but only modestly.

Most of the time, in practice, what would probably happen, is users narrowing the result before using it.

In the end, having the API allow to do this branching up front (method selection vs result narrowing) seems very reasonable.

I don't see a "best" option here. Maybe both, e.g.:

graphql.execute(...) // ExecutionResult | Promise<AsyncIterator<ExecutionResult>
graphql.executeSubscription() // Promise<AsyncIterator<ExecutionResult>
graphql.executeQuery() // ExecutionResult
graphql.executeMutation() // ExecutionResult

I don't know if splitting Query / Mutation for API symmetry has any functional benefit here. Maybe that's acceptable, not sure.

@jamiter
Copy link

jamiter commented Nov 9, 2023

With the introduction of @defer and @stream the execute function actually can return an async iterator. This might be a great time to also allow subscription execution via the execute function.

@yaacovCR
Copy link
Contributor

With the introduction of @defer and @stream the execute function actually can return an async iterator. This might be a great time to also allow subscription execution via the execute function.

Note: with the proposed new format for incremental delivery and the (temporary?) disabling of incremental delivery with regard to subscriptions, there remains a substantial division between the return types of execute and subscribe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants