Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RouteQueryManager doesn't play nice with loading substate #67

Open
bgentry opened this issue Oct 30, 2017 · 7 comments
Open

RouteQueryManager doesn't play nice with loading substate #67

bgentry opened this issue Oct 30, 2017 · 7 comments

Comments

@bgentry
Copy link
Member

bgentry commented Oct 30, 2017

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
Copy link
Member Author

bgentry commented Oct 30, 2017

This is a similar edge case to the one encountered by @viniciussbs during development of RouteQueryManager: #20 (comment)

@bgentry
Copy link
Member Author

bgentry commented Oct 30, 2017

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 #20 (comment).

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 emberjs/ember.js#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.

@viniciussbs
Copy link
Contributor

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

@villander
Copy link
Contributor

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

@bgentry
Copy link
Member Author

bgentry commented Nov 5, 2017

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.

@solocommand
Copy link

@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();
    }
  },

@bgentry
Copy link
Member Author

bgentry commented Mar 22, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants