nickiaconis / ember-prefetch

An Ember.js addon that enables parallel data requests.
http://nickiaconis.github.io/ember-parallel-model-demo/
MIT License
61 stars 12 forks source link

Redirecting during prefetch undermines Ember routing behavior #101

Open nickiaconis opened 6 years ago

nickiaconis commented 6 years ago

What's the issue?

The downstream case works fine—the prefetch hooks of routes farther from the root than a route that starts a new transition during its prefetch hook will not be called because the current transition enters the aborted state. There is no desire to preserve the model of downstream routes because they are never encountered.

Conversely, the upstream case should, but does not, account for preservation of the model. Ember's routing system establishes that redirection in a downstream route does not affect the resolution of upstream routes' models. However, when an app starts a new transition from within a prefetch hook, it invalidates all of the upstream routes' models. This causes unnecessary API requests as the app churns through the upstream routes' prefetch hooks multiple times.

I propose we disallow—or at the very least discourage—redirects during the prefetch hook.

What are some arguments against this?

Arguments that support creating new transitions during the prefetch hook likely stem from the desire to fail early. This is a commendable goal, but there are other ways to achieve early failure, such as moving the redirect logic into the route closest to the root where information is still sufficiently available to make the call whether to redirect or not. The redirect logic can then make use of the traditional model/redirect hooks, ensuring it does not invalidate the models of upstream routes, and giving upstream routes the opportunity to use the traditional hooks as well.

How do we teach this?

This will need to be documented in the README.

Ideally, we can prevent this behavior by calling the prefetch hook with a modified context that blocks access to the transition API or throws an error when an app attempts to use it. This will require a major version increment. Leading up to this, a deprecation can be added that warns of this breaking change and encourages application developers to disuse the transition API in the prefetch hook.

nickiaconis commented 6 years ago

@stefanpenner @rwjblue I'd like to get your opinions on this

stefanpenner commented 6 years ago

@nickiaconis its not super clear to me that we should allow/encourage transitions during prefetch. That seems suspect to me. Could you share some use-cases/motivations that require this?

nickiaconis commented 6 years ago

My intention is actually the exact opposite: to disallow/discourage transitions during prefetch. Sorry for the confusion @stefanpenner. Maybe including some specific examples in the description would have helped drive that fact home.

stefanpenner commented 6 years ago

My intention is actually the exact opposite: to disallow/discourage transitions during prefetch.

As you can likely tell by my above comment, this direction sounds very reasonable to me.

Would also like to hear @rwjblue's thoughts.

chriskrycho commented 4 years ago

If anyone else happens to run into this: in at least some cases, prefetch hook invocations which include transitionTo without aborting the current transition will fail on Ember 3.6+ (i.e. with the updated, public RouterService). The failure mode is that the transitions simply never resolve and the app hangs.