updateQuery of subscribeToMore should be optional #879
Conversation
|
@Torsten85: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/ |
| @@ -228,7 +228,7 @@ export class ObservableQuery extends Observable<ApolloQueryResult> { | |||
| variables: options.variables, | |||
| }); | |||
|
|
|||
| const reducer = options.updateQuery; | |||
| const reducer = options.updateQuery || ((previousResult) => previousResult); | |||
There was a problem hiding this comment.
I think instead of this we should just not call this.updateQuery on line 243 if options.updateQuery is not passed.
There was a problem hiding this comment.
Wouldn't the complete next function be obsolete if updateQuery is not passed? Seems like next is optional.
https://github.com/apollostack/apollo-client/blob/master/src/util/Observable.ts#L38
There was a problem hiding this comment.
I think you're right! But we still need to call subscribe because otherwise the subscription is not started.
… updateQuery function
|
@stubailo could you have another look. |
|
@Torsten85 I can review it, but can you merge in the changes from master first? We added a couple of lines of code there. |
|
This pull request should now be up to date with the current master branch. |
|
@Torsten85 I think it's missing a test that shows that |
|
@Torsten85 are you still interested in this PR? 🙂 |
|
I'm trying to put some order into issues and PRs, so I'm going to close this one for lack of activity, but that doesn't mean that I'm not interested in seeing the feature implemented eventually. |
As mentioned in #871, the updateQuery function can no be covered with the new graphql options.reducer. So it should be optional.