-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Comments
subscription
with execute
?subscribe
with execute
?
@ericls Main difference is in the return value since graphql-js/src/subscription/subscribe.js Line 77 in 0d2220f
Another big difference is that graphql-js/src/execution/execute.js Line 157 in 0d2220f
Both
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
} |
@IvanGoncharov, Thank you for you reply! But both |
@ericls Sorry for the confusion I confused it with: graphql-js/src/utilities/getOperationAST.js Lines 14 to 17 in 0d2220f
It's pretty basic validation (I thought you mean full query validation made by I agree that in theory, it would be great to unify the execution of So it's something that we case explore post |
@IvanGoncharov , thanks again for you reply.
|
Just happened this use-case. As a user, my initial expectation was that I would be able to do 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. |
With the introduction of |
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. |
Right now, subscribe and execute are two functions. And there are some issues with that.
execute
orsubscribe
based on Operation's type, and they go through a similar validation, resulting in duplicated work.The text was updated successfully, but these errors were encountered: