Automatic unsubscribes: new query manager mixins for routes + components #20

bgentry · 2017-05-01T02:30:28Z

This is an attempt at solving #19.

These mixins are intended to replace most direct usage of the Apollo service. The query managers act as a proxy to the Apollo service, tracking all watched queries made in a route or component and automatically unsubscribing from them via the respective lifecycle hooks.

Additionally, the objects returned by watchQuery include an _apolloObservable property that gives direct access to the ObservableQuery returned by ApolloClient.watchQuery. This should make it easy to refetch queries or handle paginated result sets.

This is poorly tested so far! I'm posting it now for feedback. Here's what the proposed usage would be in a route:

import ApolloRouteQueryManager from 'myapp/mixins/apollo-route-query-manager';

export default Ember.Route.extend(ApolloRouteQueryManager, {
  model() {
    return Ember.RSVP.hash({
      episodes: this.apollo.watchQuery({ query: EpisodesQuery }, 'episodes'),
      heroes: this.apollo.watchQuery({ query: HeroesQuery }, 'heroes')
    });
  }

The this.apollo in the route is not actually the apollo service; it's a proxy object that tracks any queries performed in the route, automatically unsubscribing from them when resetController is called.

The usage for components is similar.

cc @viniciussbs

bgentry · 2017-05-01T06:51:45Z

This is essentially "working" and I started changing out some of the apollo usage in my own app's routes and components. The ergonomics of actually using this feels great compared to the old way. The main drawback I've run into so far is that testing with this change feels a bit weird.

bgentry · 2017-05-01T06:53:12Z

addon/mixins/base-query-manager.js

+      owner.register('object:apollo-query-manager', QueryManager, {
+        singleton: false,
+      });
+    }


I have no idea what I'm doing here, but I had to figure something out to make this work reasonably well in unit + integration tests for routes/components where the mixins were being used. Without this section, apollo was undefined in routes and components.

Rather than having the query managers be container-managed, one alternative might be to introduce a simplecreateQueryManager method to the existing apollo service:

createQueryManager() { return QueryManager.create({ apollo: this }); }

This base mixin could then just rely on the service and instantiate a query manager on init as it's already doing, but you wouldn't have to deal with the custom registration.

Integration tests should just work, and unit tests could continue to just have needs: ['service:apollo'].

bgentry · 2017-05-01T07:20:16Z

addon/services/apollo.js

@@ -179,6 +203,23 @@ export default Service.extend({
    );
  },

+  managedWatchQuery(opts, resultKey, manager) {
+    let observable = this.client.watchQuery(opts);
+    manager.get('queries').pushObject(observable);


There should be explicit interfaces between the service and the manager being called here, such as addObservableQuery() and addSubscription()

bgentry · 2017-05-02T05:07:44Z

app/mixins/apollo-component-query-manager.js

@@ -0,0 +1 @@
+export { default } from 'ember-apollo-client/mixins/component-query-manager';


Is it a best practice to avoid putting mixins in app/ when users can just import them from ember-apollo-client/mixins/{name}?

viniciussbs · 2017-05-10T00:06:27Z

addon/services/apollo.js

@@ -26,7 +27,7 @@ function newDataFunc(observable, resultKey, resolve) {
  let mergedProps = { _apolloObservable: observable };

  return ({ data }) => {
-    let dataToSend = isNone(resultKey) ? data : data[resultKey];
+    let dataToSend = isNone(resultKey) ? data : get(data, resultKey);


I was thinking about to send a PR with this change. With get, I guess I can get a nested property

return this.get('apollo').query({ query }, 'pretty.nested.property');

if you want to PR that separately (with a test) I'll happily merge it right away. No reason it needs to wait until I finalize this PR.

viniciussbs · 2017-05-17T21:13:30Z

@bgentry I will start using this branch on my app tomorrow. I'll give you feedbacks.

bgentry · 2017-05-17T21:52:06Z

@viniciussbs ok, I still plan to make changes due to @dfreeman's feedback but it does at least work for me. Just a little tougher to set up than I'd like.

viniciussbs · 2017-05-19T20:14:31Z

addon/services/apollo.js

@@ -21,6 +22,31 @@ const {

 const { alias } = computed;

+function newDataFunc(observable, resultKey, resolve) {
+  let obj;
+  let mergedProps = { _apolloObservable: observable };


Since you are exposing the observable to be used, I think the property shouldn't be named as a private one. apolloObservable looks more appropriate to me.

My thought for keeping this private is that I wouldn't want the observable to show up in i.e. property iteration. Maybe a better, more explicit API would be to name this __apolloObservable__ to make it obvious that it's a private property, and then expose it via some accessor function:

import { getObservable } from 'ember-apollo-client'; export default Ember.Route.extend({ model() { let result = this.apollo.query(...); let observable = getObservable(result); ... } }

Thoughts on whether that's an improved design or if it's overkill? cc @viniciussbs @dfreeman

I'd agree that it's odd to have the public documented solution for something start with "grab this property that starts with an underscore...". You could use defineProperty to explicitly make it nonenumerable, though it'll of course still show up in dev tools (albeit with a slightly faded-out key).

I personally like the getObservable approach a lot, since it abstracts away the exact details of how the access works and leaves you free to change it in the future.

I kept the property named _apolloObservable, but now you're expected to get it as I documented in my proposal above. Is that sufficient, or should I make further changes (such as with defineProperty)?

I like this approach, too. The defineProperty one could be used in the future if we return something like a ApolloResult or something like that instead of a Ember.Object. The getObservable looks like the best shot right now.

viniciussbs · 2017-05-26T23:05:28Z

Hi, @bgentry.

Inside a component I have a query to fetch resources that will be paginated and another query to fetch a single resource once. For the first query, I call this.apollo.watchQuery. Nice. But, for the seconde one, I can't call this.apollo.query - or this.apollo.queryOnce, using the addon name for the function. So I'm injecting the service - with another name to avoid collision - just to use queryOnce.

The problem here is having two different syntax to use Apollo. I have this.apollo.watchQuery and this.get('apolloService').queryOnce inside the same component. It would be nice to call this.apollo.query instead.

I've suggested query because the query manager is following the vanilla API.

viniciussbs · 2017-06-15T18:56:02Z

Hi, @bgentry

Any thoughts about the things I've pointed? And are you planning a merge soon?

bgentry · 2017-06-15T20:32:10Z

@viniciussbs yes to both. Planning to make changes based on the feedback. I might be able to get back to it early next week.

As for the query/queryOnce/watchQuery thing, I'm thinking the direction I should go is toward unifying those names w/ ApolloClient itself. It will probably require a series of changes, first deprecating query with a watchQuery alias, then doing the same with queryOnce -> query.

I'm trying to make this new query manager follow ApolloClient right from the start.

Also going to make the changes @dfreeman suggested.

Sorry for the delay, my startup has been busy and I haven't been able to touch the frontend in awhile.

viniciussbs · 2017-06-17T17:46:31Z

@bgentry Nice.

As for the query/queryOnce/watchQuery thing, I'm thinking the direction I should go is toward unifying those names w/ ApolloClient itself. It will probably require a series of changes, first deprecating query with a watchQuery alias, then doing the same with queryOnce -> query.

The best choice in my opinion.

viniciussbs · 2017-07-19T18:18:39Z

addon/mixins/route-query-manager.js

+export default Mixin.create(BaseQueryManager, {
+  resetController() {
+    this._super(...arguments);
+    this.get('apollo').unsubscribeAll();


I'm facing a problem with this.

Let's say I'm on "/comments?status=approved" and then I navigate to "/posts":

the function model inside app/routes/posts.js runs this.apollo.watchQuery to fetch posts;

the query manager inside app/routes/posts.js keeps the active observable queries from this route;

the function resetController inside app/routes/comments.js runs this.get('apollo').unsubscribeAll();

the query manager inside app/routes/comments.js unsubscribe all observable query.

That's fine and working as expected. Now let's say I'm on "/comments?status=approved" and then I navigate to "/comments?status=pending":

the function model inside app/routes/comments.js runs this.apollo.watchQuery to fetch pending comments;

the query manager inside app/routes/comments.js keeps the active observable queries from this route;

the function resetController inside app/routes/comments.js runs this.get('apollo').unsubscribeAll();

the query manager inside app/routes/comments.js unsubscribe all observable query, including the one that was just pushed to the query manager.

The problem with this second scenario is noticed when I want to fetch more comments. The function fetchMore is called on the observable query that was previously unsubscribed and then Apollo tells me that the query does not exist.

I'm handle this using a copy of this mixin that only unsubscribe when we are exiting:

resetController(_controller, isExiting, _transition) { this._super(...arguments); if (isExiting) { this.get('apollo').unsubscribeAll(); } }

@viniciussbs ok, so I think I understand this scenario. When switching from /comments?status=approved to /comments?status=pending, you do in fact want for the "approved comments" subscription to be ended. However, resetController()/unsubscribeAll() isn't called until after the new model is loaded. Therefore both the "approved comments" and "pending comments" subscriptions are ended because they are both tracked at the time unsubscribeAll() is called.

Is there maybe another route hook I should be utilizing to mark when a transition has begun? That would potentially let me flag all subscriptions which existed prior to the new model data, such that I would only clear things that existed before.

@viniciussbs I just pushed up my progress thus far, including a new commit with a failing test case that I believe covers what you described. Can you please confirm before I write the fix?

My current plan is to utilize the beforeModel() route hook to tell the query manager to mark all pre-existing queries as stale, and then when resetController() gets called later, it should only cancel subscriptions that were marked as stale. Does that seem like the correct behavior and the best way to implement it?

Sorry for the late response, @bgentry

Yes, this is the scenario. I don't know a more appropriate hook for this case. Your plan looks like the ideal solution for me.

viniciussbs · 2017-07-29T02:09:41Z

@bgentry Could you rebase against master? I would love to save my queries and mutations in external files.

bgentry · 2017-07-29T18:01:06Z

@viniciussbs sorry for how long it's taken me to get back to this. Haven't been working on the front-end parts of my app much for the past couple months, and was also busy getting married :)

Will be taking this across the finish line soon.

viniciussbs · 2017-07-29T21:49:27Z

@bgentry Oh, congratulations! Best wishes for you.

If you need any help, let me know.

villander · 2017-08-14T11:21:11Z

@bgentry What's missing for this branch to be merge? I want to help

viniciussbs · 2017-09-01T13:40:58Z

@bgentry Great news! :-) If you need any help, let me know. And thank you for creating a new branch.

@viniciussbs

As noted[1] by @viniciussbs, there's a flaw in this implementation. When `model()` is called twice on a route due to a query params change, all watchQuery subscriptions are cancelled, including the brand new one with the updated query params. Really we should only be cancelling the old queries that existed prior to the model reload. [1]: #20 (comment)

@viniciussbs

As noted[1] by @viniciussbs, there's a flaw in this implementation. When `model()` is called twice on a route due to a query params change, all watchQuery subscriptions are cancelled, including the brand new one with the updated query params. Really we should only be cancelling the old queries that existed prior to the model reload. [1]: #20 (comment)

bgentry · 2017-09-03T23:57:54Z

I pushed up all my progress so far. I think all issues are resolved aside from the issue raised by @viniciussbs about query params and multiple model() calls, which I've written a failing test case for. I also need to update all documentation and the readme to reflect these changes.

These mixins are intended to replace most direct usage of the Apollo service. The query managers act as a proxy to the Apollo service, tracking all watched queries made in a route or component and automatically unsubscribing from them via the respective lifecycle hooks. Additionally, the objects returned by watchQuery include an `_apolloObservable` property that gives direct access to the ObservableQuery returned by ApolloClient.watchQuery. This should make it easy to refetch queries or handle paginated result sets.

For better alignment with Apollo Client itself, this addon will move towards these methods being named `query` for one-off queries and `watchQuery` for watched queries.

@viniciussbs

As noted[1] by @viniciussbs, there's a flaw in this implementation. When `model()` is called twice on a route due to a query params change, all watchQuery subscriptions are cancelled, including the brand new one with the updated query params. Really we should only be cancelling the old queries that existed prior to the model reload. [1]: #20 (comment)

bgentry · 2017-09-05T00:11:28Z

I realized I can't actually deprecate _apolloUnsubscribe just yet, as it's still the only mechanism for unsubscribing for data fetched outside a route or component (such as within a service). Going to just punt on that issue for now and remove the deprecation warning.

Rather than documenting and expecting users to use a private-ish key to unsubscribe from a watchQuery, expose a function whose explicit purpose is to get the observable. This makes it easier to change the implementation in the future while keeping the same API.

When re-fetching a model (likely due to a change in query parameters), we only want to end any old ("stale") subscriptions that existed prior to the new model(s) being loaded. To do this, we mark preexisting subscriptions as stale when `beforeModel` is called. Additionally, we need to clear out *all* subscriptions when the route is being torn down, so we special case this scenario in the RouteQueryManager.

This is handled automatically now by the RouteQueryManager mixin.

bgentry · 2017-09-05T00:41:29Z

Alright, I think that should cover everything! Last call for review or feedback. I've updated the readme to reflect new usage. I'll also be updating my own app to use this updated branch as a final check.

This will be released as v0.4.0 as it includes a couple of new deprecations, though nothing breaking unless you were already using an earlier version of this PR.

bgentry · 2017-09-05T05:09:50Z

Well, it took me just a bit too long to come back to this PR. Sorry for the delays, but it's now merged. Releasing v0.4.0 shortly!

viniciussbs · 2017-09-05T13:56:26Z

Thank you for your efforts, @bgentry. I'll try to update to v0.4.0 today.

bgentry force-pushed the automatic-unsubscribes branch 5 times, most recently from 62ea456 to 73cd107 Compare May 1, 2017 06:48

bgentry commented May 1, 2017

View reviewed changes

bgentry mentioned this pull request May 1, 2017

How should we do pagination? #18

Closed

bgentry force-pushed the automatic-unsubscribes branch from 73cd107 to a9d74ac Compare May 1, 2017 23:25

bgentry commented May 2, 2017

View reviewed changes

viniciussbs mentioned this pull request May 8, 2017

Update model after a mutation with refetchQueries option #26

Closed

bgentry force-pushed the automatic-unsubscribes branch 2 times, most recently from f52b779 to 210aeaf Compare May 9, 2017 23:02

viniciussbs reviewed May 10, 2017

View reviewed changes

viniciussbs reviewed May 19, 2017

View reviewed changes

bgentry mentioned this pull request Jun 28, 2017

Data returned by apollo service query does not include __typename #34

Closed

viniciussbs reviewed Jul 19, 2017

View reviewed changes

viniciussbs mentioned this pull request Jul 29, 2017

Various and Sundries Questions #36

Closed

bgentry mentioned this pull request Aug 2, 2017

Apollo Imperative Store Usable #38

Closed

bgentry force-pushed the automatic-unsubscribes branch from 210aeaf to bfcbe1f Compare August 31, 2017 21:08

bgentry force-pushed the automatic-unsubscribes branch from bfcbe1f to 9ffe7b5 Compare September 3, 2017 23:20

bgentry force-pushed the automatic-unsubscribes branch from cec7da6 to 0fd504a Compare September 3, 2017 23:56

bgentry added 6 commits September 4, 2017 15:29

use get for safety

43f949b

add watchQuery to Apollo service, deprecate query

f8479f0

For better alignment with Apollo Client itself, this addon will move towards these methods being named `query` for one-off queries and `watchQuery` for watched queries.

documentation for QueryManager

065d50b

fixup watchQuery

6fd49bb

bgentry force-pushed the automatic-unsubscribes branch from 0fd504a to 510345d Compare September 4, 2017 23:13

bgentry added 4 commits September 4, 2017 17:13

deprecate UnsubscribeRoute mixin

1012815

don't explicitly inject apollo service into routes

8a3c20d

This is handled automatically now by the RouteQueryManager mixin.

bgentry force-pushed the automatic-unsubscribes branch from 3a4e4ed to 77bae96 Compare September 5, 2017 00:37

readme updates

c04de01

bgentry force-pushed the automatic-unsubscribes branch from 77bae96 to c04de01 Compare September 5, 2017 00:56

bgentry merged commit 458ed93 into master Sep 5, 2017

bgentry deleted the automatic-unsubscribes branch September 5, 2017 05:08

bgentry mentioned this pull request Sep 5, 2017

Improved unsubscribe design #19

Closed

viniciussbs mentioned this pull request Sep 14, 2017

Apollo service is initializes during integration test without its options #52

Closed

bgentry mentioned this pull request Oct 30, 2017

RouteQueryManager doesn't play nice with loading substate #67

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatic unsubscribes: new query manager mixins for routes + components #20

Automatic unsubscribes: new query manager mixins for routes + components #20

bgentry commented May 1, 2017 •

edited

Loading

bgentry commented May 1, 2017 •

edited

Loading

bgentry May 1, 2017

dfreeman May 2, 2017

bgentry May 1, 2017

bgentry May 2, 2017

viniciussbs May 10, 2017

bgentry May 10, 2017

viniciussbs commented May 17, 2017

bgentry commented May 17, 2017

viniciussbs May 19, 2017

bgentry Sep 1, 2017

dfreeman Sep 1, 2017

bgentry Sep 3, 2017

viniciussbs Sep 4, 2017

viniciussbs commented May 26, 2017

viniciussbs commented Jun 15, 2017

bgentry commented Jun 15, 2017

viniciussbs commented Jun 17, 2017

viniciussbs Jul 19, 2017

bgentry Sep 1, 2017 •

edited

Loading

bgentry Sep 3, 2017

viniciussbs Sep 4, 2017

viniciussbs commented Jul 29, 2017

bgentry commented Jul 29, 2017

viniciussbs commented Jul 29, 2017

villander commented Aug 14, 2017

viniciussbs commented Sep 1, 2017

bgentry commented Sep 3, 2017

bgentry commented Sep 5, 2017

bgentry commented Sep 5, 2017

bgentry commented Sep 5, 2017

viniciussbs commented Sep 5, 2017

		@@ -0,0 +1 @@
		export { default } from 'ember-apollo-client/mixins/component-query-manager';

Automatic unsubscribes: new query manager mixins for routes + components #20

Automatic unsubscribes: new query manager mixins for routes + components #20

Conversation

bgentry commented May 1, 2017 • edited Loading

bgentry commented May 1, 2017 • 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

viniciussbs commented May 17, 2017

bgentry commented May 17, 2017

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

viniciussbs commented May 26, 2017

viniciussbs commented Jun 15, 2017

bgentry commented Jun 15, 2017

viniciussbs commented Jun 17, 2017

Choose a reason for hiding this comment

bgentry Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viniciussbs commented Jul 29, 2017

bgentry commented Jul 29, 2017

viniciussbs commented Jul 29, 2017

villander commented Aug 14, 2017

viniciussbs commented Sep 1, 2017

bgentry commented Sep 3, 2017

bgentry commented Sep 5, 2017

bgentry commented Sep 5, 2017

bgentry commented Sep 5, 2017

viniciussbs commented Sep 5, 2017

bgentry commented May 1, 2017 •

edited

Loading

bgentry commented May 1, 2017 •

edited

Loading

bgentry Sep 1, 2017 •

edited

Loading