GraphQL websocket subscription integration #540
Conversation
|
@amandajliu @stubailo when this is ready, can we publish it as a rc so I can prep the react client to work with it? |
|
also, SO excited for this! |
| @@ -5,6 +5,7 @@ Expect active development and potentially significant breaking changes in the `0 | |||
| ### vNEXT | |||
| - Fixed an issue with named fragments in batched queries. [PR #509](https://github.com/apollostack/apollo-client/pull/509) and [Issue #501](https://github.com/apollostack/apollo-client/issues/501). | |||
| - Fixed an issue with unused variables in queries after diffing queries against information available in the store. [PR #518](https://github.com/apollostack/apollo-client/pull/518) and [Issue #496](https://github.com/apollostack/apollo-client/issues/496). | |||
| - Add code that support GraphQL subscription over websockets. [PR #540](https://github.com/apollostack/apollo-client/pull/540). | |||
There was a problem hiding this comment.
Typo. Maybe replace "that" with "to" and subscription with subscriptions.
|
Awesome work! I think subscriptions are going to be such a great addition to apollo-client. The only thing I'm concerned about is that the tests don't cover enough potential cases yet, but that can be fixed later. @stubailo Do you know what happened to coveralls in apollo-client? I can't see it in the CI checks any more. |
| @@ -89,17 +89,21 @@ export function addQueryMerging(networkInterface: NetworkInterface): BatchedNetw | |||
| }) as BatchedNetworkInterface; | |||
| } | |||
|
|
|||
| // Here, we are assuming that there is only one operation | |||
| function getSingleOperationName(request: Request) { | |||
There was a problem hiding this comment.
I'll add a check here just to make sure there is actually only one operation.
|
@stubailo this looks good to me. I think I'll just add one check for the assumption that there's only one operation definition. Can you give it a quick look to make sure it fits into apollo client the way it's supposed to (and uses argument variable names that make sense in the context of AC)? |
| @@ -5,6 +5,7 @@ Expect active development and potentially significant breaking changes in the `0 | |||
| ### vNEXT | |||
| - Fixed an issue with named fragments in batched queries. [PR #509](https://github.com/apollostack/apollo-client/pull/509) and [Issue #501](https://github.com/apollostack/apollo-client/issues/501). | |||
| - Fixed an issue with unused variables in queries after diffing queries against information available in the store. [PR #518](https://github.com/apollostack/apollo-client/pull/518) and [Issue #496](https://github.com/apollostack/apollo-client/issues/496). | |||
| - Add code to support GraphQL subscriptions over websockets. [PR #540](https://github.com/apollostack/apollo-client/pull/540). | |||
There was a problem hiding this comment.
"websockets" doesn't belong here.
| variables: graphQLSubscriptionOptions.variables, | ||
| fragments: graphQLSubscriptionOptions.fragments, | ||
| handler: (error: Object, result: Object) => { | ||
| const reducer = graphQLSubscriptionOptions.updateFunction; |
There was a problem hiding this comment.
I think this should be updateQuery, just like fetchMore. Sorry I didn't check on this.
There was a problem hiding this comment.
Ok, I can change it for 0.4.13. Heads up @jbaxleyiii
|
I noticed you have to pass an id to unsubscribe, can you guys make it so that you can just call stop on the subscription object? |
|
This is exciting though! |
|
Yeah probably this is what we will want in the future - subscriptions should probably return Observables, the same way We are trying to get the experimental thing out the door fast and then iterate on the API. |
TODO: