ember-apollo-client icon indicating copy to clipboard operation
ember-apollo-client copied to clipboard

RouteQueryManager doesn't play nice with loading substate

Open bgentry opened this issue 8 years ago • 7 comments

I have a route in my app called widgets.show that takes an ID param. I tried adding a loading substate to my app with a template named show-loading.hbs. Once I add this template and I transition between showing different widgets (updating the :id route param from i.e. 1 to 2), the following sequence of events happens:

  1. beforeModel() is called on the widgets.show route for the new ID 2. This causes the RouteQueryManager to mark all preexisting watch query subscriptions as stale, which is good. In this case, it's only the query for widget with ID 1 that is active and thus marked stale.
  2. model() gets called with the new ID 2, initiating the Apollo Client request and starting a subscription which is not stale.
  3. resetController() is called on the widgets.show route with isExiting=true. The RouteQueryManager then unsubscribes to all queries, which includes the pending query for the new widget. The transition argument to resetController() is null.
  4. The new widget route never leaves the loading state because its promise is never resolved (due to the subscription being unsubscribed).

The problem here is that RouteQueryManager uses isExiting to determine whether it should unsubscribe to all queries, or just those that were previously marked as stale. With a loading route, the previous widgets.show route is exited in favor of widgets.show_loading. If I remove the loading template, isExiting remains false and only the stale queries are removed.

I'm not sure right now how to solve this. Any ideas?

bgentry avatar Oct 30 '17 00:10 bgentry

This is a similar edge case to the one encountered by @viniciussbs during development of RouteQueryManager: https://github.com/bgentry/ember-apollo-client/pull/20#discussion_r128326261

bgentry avatar Oct 30 '17 05:10 bgentry

I think I have a solution in mind for this, however it might be blocked by an Ember bug.

This line in RouteQueryManager decides whether or not to unsubscribe all known queries, or just those that have already been marked stale. This was necessary to handle the issue encountered by @viniciussbs in https://github.com/bgentry/ember-apollo-client/pull/20#discussion_r128326261.

However, this appears to be flawed when using loading substates, because Ember treats those as an actual route transition in that resetControllergets called with isExiting=true. A simple solution that should work is to only ever clear stale subscriptions in resetController, but clear all known subscriptions on deactivate. Deactivate should only get called when the route is actually changing.

The problem with this is that that's not what actually happens with deactivate. Sometime around Ember 2.9 its behavior was changed (perhaps unintentionally) such that deactivate does in fact get called on transitions between the same route when there is a loading substate. It's documented in https://github.com/emberjs/ember.js/issues/15466.

Might not be able to fix this issue until that's fixed, unless somebody comes up with a better solution.

If we can get confirmation that that's indeed a bug, we should actually be able to make these changes ahead of time and just have loading routes continue to malfunction (the same as they are today) until the Ember bug is fixed.

bgentry avatar Oct 30 '17 06:10 bgentry

@villander Could to talk to @locks to check this possible bug in Ember?

viniciussbs avatar Oct 31 '17 23:10 viniciussbs

Yes this is the bug, we can either wait for this bug to be resolved or send a PR to Ember.js

villander avatar Nov 01 '17 01:11 villander

So I totally misunderstood that Ember issue and I don't think it's related. I guess it's expected behavior that if you transition from /posts/1 to /posts/2, your app will transition from posts.show to posts.show-loading and then back to posts.show again.

This is problematic for the design of the RouteQueryManager because of the order in which hooks are executed, and the fact that none of the hooks seem to provide enough info about the transition for the query manager to make the right decisions.

Still searching for a solution to this.

bgentry avatar Nov 05 '17 19:11 bgentry

@bgentry From my testing it seems like the transition is undefined when making a transition to the same route (changing between substates like loading, or via query params).

To bypass this issue, I'm overriding the resetController method from the RouteQueryManager and only performing the unsubscribe when there is a transition, and we're not going to an error substate:



  resetController(_controller, isExiting, transition) {
    if (isExiting && transition && transition.targetName !== 'error') {
      this.get('apollo').unsubscribeAll();
    }
  },

solocommand avatar Mar 22 '18 20:03 solocommand

I never updated this thread, but I did speak to @rwjblue about this at one point. He said there was really no workaround or fix for this other than to not use loading substates.

bgentry avatar Mar 22 '18 22:03 bgentry