subscribeToMore for observableQuery #797

helfer · 2016-10-18T01:18:02Z

TODO:

If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
Make sure all of the significant new logic is covered by tests
Rebase your changes on master so that they can be merged easily
Make sure all tests and linter rules pass
Update CHANGELOG.md with your change
Add your name and email to the AUTHORS file (optional)
If this was a change that affects the external API, update the docs and post a link to the PR in the discussion
add a test that shows subscriptions are cancelled when the ObservableQuery is removed.

stubailo · 2016-10-18T04:36:07Z

CHANGELOG.md

@@ -24,6 +24,7 @@ Expect active development and potentially significant breaking changes in the `0
 - Fix multidimentional array handling. [Issue #776](https://github.com/apollostack/apollo-client/issues/776) [PR #785](https://github.com/apollostack/apollo-client/pull/785)
 - Add support for Enum inline arguments [Issue #183](https://github.com/apollostack/apollo-client/issues/183) [PR #788](https://github.com/apollostack/apollo-client/pull/788)
 - Make it possible to subscribe to the same observable query multiple times. The query is initialized on the first subscription, and torn down after the last. Now, QueryManager is only aware of one subscription from the ObservableQuery, no matter how many were actually registered. This fixes issues with `result()` and other ObservableQuery features that relied on subscribing multiple times to work. This should remove the need for the workaround in `0.4.21`. [Repro in PR #694](https://github.com/apollostack/apollo-client/pull/694) [PR #791](https://github.com/apollostack/apollo-client/pull/791)
+- ** new Feature **: Add fetchMore-style subscribeToMore function which updates a query result based on a subscription. [PR #797](https://github.com/apollostack/apollo-client/pull/797)


I don't think you're allowed to put spaces between the ** like this in markdown.

that's funny, because I think I copy-pasted it from another entry :D Will fix both.

stubailo · 2016-10-18T04:36:41Z

src/core/ObservableQuery.ts

@@ -165,7 +168,8 @@ export class ObservableQuery extends Observable<ApolloQueryResult> {
        const reducer = fetchMoreOptions.updateQuery;
        const mapFn = (previousResult: any, { variables }: {variables: any }) => {

-          // TODO REFACTOR: reached max recursion depth (figuratively). Continue renaming to variables further down when we have time.
+          // TODO REF: reached max recursion depth (fig) when renaming queryVariables to variables.


I don't think using abbreviations in comments is a good idea.

stubailo · 2016-10-18T04:36:54Z

src/core/ObservableQuery.ts

@@ -178,6 +182,49 @@ export class ObservableQuery extends Observable<ApolloQueryResult> {
      });
  }

+  // XXX the subscription variables are separate from the query variables.
+  // if you want to update subscription variables, right now you have to do that separately,
+  // and you can only do it by stopping the subscription and then subscribing again with new variables.


stubailo · 2016-10-18T04:38:33Z

src/core/ObservableQuery.ts

+      // XXX technically we should also remove it from this.subscriptionHandles
+      const i = this.subscriptionHandles.indexOf(subscription);
+      if (i >= 0) {
+        this.subscriptionHandles.splice(i, 1);


Is the above comment about removing it out of date?

it is indeed

isn't splice removing it?
maybe there is a typo in the comment?

stubailo · 2016-10-18T04:39:04Z

src/core/ObservableQuery.ts

+
+    this.subscriptionHandles.push(subscription);
+
+    return () => {


If we later want to return more stuff like the errors, we will wish we didn't just return a function here.

the convention is to return observable, then right after use observer.error(error)

stubailo · 2016-10-18T04:39:54Z

src/core/watchQueryOptions.ts

+  document: Document;
+  variables?: { [key: string]: any };
+  updateQuery: (previousQueryResult: Object, options: {
+    subscriptionData: any,


Should we just wrap this in a data: ... to keep it consistent with fetchMore? I feel like that would be better than having two different but very similar APIs.

i agree with you that both should be consistent..

yeah, I wasn't sure where to do it. I actually did that in another place, but I'd rather solve it in the subscriptions themselves. Will do the "patching" for now.

DxCx · 2016-10-18T05:45:12Z

src/core/ObservableQuery.ts

@@ -80,6 +82,7 @@ export class ObservableQuery extends Observable<ApolloQueryResult> {
    this.queryId = queryId;
    this.shouldSubscribe = shouldSubscribe;
    this.observers = [];
+    this.subscriptionHandles = [];
  }

  public result(): Promise<ApolloQueryResult> {


When converting Observables to promises the convention is usually to take last value emitted, not first value emitted.
which means you should save last value emitted, and only when complete reach resolve it for the promise.
also, i would have a global util function to do that task, and then just call it here.

I don't think that should go as a comment on this PR, since that code was not modified at all. Perhaps open a new issue or a PR? But I'm not sure how following that convention will improve the result method.

yeah you are right. (regarding commenting here)
it will be improved because:

Observable to promise is something that alot of people do, there is a way to do it.

it should be one logic used towards the whole project, doing it over and over is redundant.
want me to open an issue for it and later on check that?

DxCx · 2016-10-18T05:48:58Z

src/core/ObservableQuery.ts

@@ -342,6 +389,9 @@ export class ObservableQuery extends Observable<ApolloQueryResult> {
      this.scheduler.stopPollingQuery(this.queryId);
    }

+    // stop all active GraphQL subscriptions
+    this.subscriptionHandles.forEach( sub => sub.unsubscribe() );


i would re-init subscriptionHandlers into an empty array at that point...
i mean, technicality, it is not needed as the teardown function cleans it.
but for people that are not familiar with observables i would add an explicit re-init
so it will be clear that it's empty at that point.

DxCx · 2016-10-18T05:54:08Z

src/core/watchQueryOptions.ts

+  document: Document;
+  variables?: { [key: string]: any };
+  updateQuery: (previousQueryResult: Object, options: {
+    subscriptionData: any,


i agree with you that both should be consistent..

DxCx · 2016-10-18T05:57:01Z

test/subscribeToMore.ts

+
+    setTimeout(() => {
+      sub.unsubscribe();
+      assert.equal(counter, 3);


I'm not sure exceptions will raise (and not just print) that way (setTimeout, annon func)
did you verify that?

Yeah, it worked alright.

cool, i have bad experience with timeouts and exceptions hehe..
better safe then sorry ;)

DxCx · 2016-10-18T07:01:31Z

src/core/ObservableQuery.ts

+  // XXX the subscription variables are separate from the query variables.
+  // if you want to update subscription variables, right now you have to do that separately,
+  // and you can only do it by stopping the subscription and then subscribing again with new variables.
+  public subscribeToMore(


i think it should return observable not just teardown function.
which means, wrap it with: return new Observable((observer) => {
observable.subscribe(map...., observer.error, observer.complete)
return () => ....
}

I thought about it but decided against it, because I think if people want access to the subscription, they should use the client.susbcribe method.

so the whole point of this function is to update the store so the orginal observable will get updates?

That's correct.

yeah, that's pretty much it :)

stubailo · 2016-10-18T18:09:33Z

I filed an issue for the one big outstanding task: #803

helfer added 4 commits October 17, 2016 15:38

first pass at subscribeToMore

8ab63ce

add a simple test

77bc255

update CHANGELOG.md

656c3ab

prevent leak with subscription handles

81a17da

stubailo reviewed Oct 18, 2016

View reviewed changes

DxCx reviewed Oct 18, 2016

View reviewed changes

helfer and others added 3 commits October 18, 2016 00:03

fix markdown in CHANGELOG.md

4e433bb

fix comments

181836c

Merge branch 'master' into subs-api

e63b397

stubailo merged commit 8b2d41f into master Oct 18, 2016

helfer mentioned this pull request Oct 26, 2016

Ideas for additional subscription APIs #644

Closed

jbaxleyiii deleted the subs-api branch July 20, 2017 17:40

github-actions bot locked as resolved and limited conversation to collaborators Feb 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

subscribeToMore for observableQuery #797

subscribeToMore for observableQuery #797

helfer commented Oct 18, 2016 •

edited

Loading

stubailo Oct 18, 2016

helfer Oct 18, 2016

stubailo Oct 18, 2016

stubailo Oct 18, 2016

stubailo Oct 18, 2016

helfer Oct 18, 2016

DxCx Oct 18, 2016

stubailo Oct 18, 2016

DxCx Oct 18, 2016 •

edited

Loading

stubailo Oct 18, 2016

DxCx Oct 18, 2016

helfer Oct 18, 2016

DxCx Oct 18, 2016

stubailo Oct 18, 2016

DxCx Oct 18, 2016

DxCx Oct 18, 2016

DxCx Oct 18, 2016

DxCx Oct 18, 2016

helfer Oct 18, 2016

DxCx Oct 18, 2016

DxCx Oct 18, 2016

helfer Oct 18, 2016

DxCx Oct 18, 2016

stubailo Oct 18, 2016

helfer Oct 18, 2016

stubailo commented Oct 18, 2016

subscribeToMore for observableQuery #797

subscribeToMore for observableQuery #797

Conversation

helfer commented Oct 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DxCx Oct 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stubailo commented Oct 18, 2016

helfer commented Oct 18, 2016 •

edited

Loading

DxCx Oct 18, 2016 •

edited

Loading