-
-
Notifications
You must be signed in to change notification settings - Fork 405
[Route Prefetching] Allow routes to request data in parallel #97
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
Changes from all commits
3d09f86
8952f05
3f8ee49
fae441c
b6b261c
79078ba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| - Start Date: 2015-10-01 | ||
| - RFC PR: (leave this empty) | ||
| - Ember Issue: (leave this empty) | ||
|
|
||
| # Summary | ||
|
|
||
| Ember's data fetching is sequential. | ||
| Although this has advantages (one of which being that it presents a simpler programming model), the resulting latency of each request is compounded and can result in a degraded experience. | ||
| The general solution to the "waterfall" problem of sequential requests is to execute independent requests in parallel. | ||
|
|
||
| Automaticly inferring which requests can be made in parallel is tricky, and likely impossible to achieve while maintaining a semver compatible public API. | ||
| Although it may not be automatic, developers have sufficient context with which they can decide what requests to make in parallel. | ||
|
|
||
| This RFC proposes an approach that allows developers to tune the balance between sequential and parallel requests. | ||
|
|
||
| # Motivation | ||
|
|
||
| Currently, the `model` hook is Ember's way for routes to request data. | ||
| However, a route's `model` hook is not entered until it's parent route has resolved. | ||
| This is suboptimal for nested application structures. | ||
| Child routes are forced to wait for their parents to receive data before they may make requests for their own data. | ||
| This occurs even when the child doesn't make use of its parents' data. | ||
|
|
||
| This RFC proposes adding a new `Route#prefetch` hook, which is used to allow routes to make requests in parallel. | ||
| Child routes that make use of `prefetch` will resolve faster since their data can resolve at the same time as (or before) their parents'. | ||
| The semantics and ordering of `Route`'s existing model hooks (`beforeModel`, `model`, `afterModel`) are preserved. | ||
|
|
||
| See this demo for a visualization: http://nickiaconis.github.io/ember-parallel-model-demo/ | ||
|
|
||
| # Detailed design | ||
|
|
||
| A `prefetch` hook is added to `Route`. | ||
| It takes the same parameters as the `model` hook. | ||
| Like the `model` hook, it is not called if an object is passed to the transition. | ||
|
|
||
| ```javascript | ||
| App.PostRoute = Ember.Route.extend({ | ||
| prefetch(params) { | ||
| return Ember.$.get(`/api/posts/${params.id}`); | ||
| } | ||
| }); | ||
|
|
||
| App.PostCommentsRoute = Ember.Route.extend({ | ||
| prefetch(params, transition) { | ||
| return Ember.$.get(`/api/posts/${transition.params.post.id}/comments`); | ||
| } | ||
| }); | ||
| ``` | ||
|
|
||
| The default functionality of the `model` hook is modified to return the prefetched data if it exists. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm unsure if we should modify the existing/default given the following scenario: // routes/user.js
export default Route.extend({
prefetch(params) {
return RSVP.hash([
user: this.store.find('user', params.user_id),
friends: this.store.find('friends', { user_id: params.user_id }),
somethingElse: ajax(...)
})
},
// the default model functionality of `this.store.find('user', params.user_id)`
// will "just work" and the ED identify map would benefit from the existing fetch.
})I would imagine, adding / removing prefetch request would be common, as the the UI changes or the data topology / size changes, tuning will occur. In addition, query-params may or may not go through the pre-fetch if model refreshes. If the model wants access to the prefetched data, it should likely be explicit. export default Route.extend({
prefetch() { ... }
model() {
return this.prefetched();
}
})
``
```js
export default Route.extend({
prefetch() { ... }
model() {
return this.prefetched('other-route');
}
})It almost seems like This would also allow an advanced user to create additional async dependencies while maybe for example: export default Route.extend({
prefetch() {
// although this now creates a sequence, prefetch firing concurrently at the start of routing,
// enables these to speed ahead, but truthfully support inter-route prefetching dependencies.
return this.prefetched('otherRoute').then(other => {
return this.store.find('user', other.id);
})
}
})
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm beginning to come around again on the idea of I worry that a However, someone who would to use the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still think the this.prefetched(x) or similar is an important part of this pattern, would be open to persuasion .. maybe thoughts have changed since these comments were made
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, thoughts have changed. The ember-prefetch add-on even implements this.prefetched(x) now, but it would appear I never updated the RFC. Will fix. |
||
| As such, a route that defines a `prefetch` hook is not required to define a `model` hook. | ||
|
|
||
| A `prefetched` method, which takes an optional `name` parameter and always returns a promise, is added to `Route`. | ||
| It is used to access data fetched by the named route's `prefetch` hook. | ||
| If `name` is omitted, the method will return a promise for its own route's prefetched data. | ||
| The default `model` hook utilizes it in this way. | ||
|
|
||
| ```javascript | ||
| App.PostCommentsRoute = Ember.Route.extend({ | ||
| prefetch(params, transition) { | ||
| return Ember.$.get(`/api/posts/${transition.params.post.id}/comments`); | ||
| }, | ||
|
|
||
| async model() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ^
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think that Yehuda has the one that first suggested this, proposing something alike to Couldn't just adding I think that this would add the very minimum cognitive syntax and cognitive overhead to the existing model. I am assuming that routes will be triggered in parallel but within each route the beforeModel/model/afterModel remain blocking to each other.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cibernox The way this is written right now, execution would go like: prefetch1, prefetch2, beforeModel1, model1, afterModel1, beforeModel2, model2, afterModel2. The async function syntax essentially turns a function into a generator, which yields a promise each place there is an await. So adding async in front of the model hook will basically make it return a promise, which Ember already assumes it does. It will not cause it to execute outside of the promise resolution chain that is a part of |
||
| return { | ||
| OP: this.modelFor('post')).author, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. extra parenthesis
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need linting for code blocks in markdown. 😛 |
||
| comments: await this.prefetched() | ||
| }; | ||
| } | ||
| }); | ||
| ``` | ||
|
|
||
| # Drawbacks | ||
|
|
||
| - Ember's API becomes larger. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it may be confusing to the programming model |
||
|
|
||
| # Alternatives | ||
|
|
||
| - Implement the functionality as an addon. | ||
| - Would require Ember to provide something like `Router#willTransition` that is triggered on redirects. It could be triggered on either all transitions (`Router#willChangeURL`?) or only redirects (`Router#changeTransition`? to supplement `willTransition`). | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it seems like these hooks are required regardless, even if this where part of ember no?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's possible that it could be implemented without these hooks, but it's doubtful that it would be preferred. Doing so would likely involve tight coupling with the router, which is no bueno. Since these hooks are useful for other purposes too (accessibility, advanced tracking, etc), I'd say they're necessary regardless. |
||
| - Otherwise, the addon must change the functionality of `Router#willTransition` in order to function properly. | ||
| - Would benefit from guarantees of stability around `Transition#handlerInfos` and `handlerInfo#runSharedModelHook`. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if prefetch cause become part of the chain, essentially invoked eagerly, but its completion blocks beforeModel on that route. This may actually be least surprising. (maybe, needs thought)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds kinda like what @tomdale has described below, though he suggests it replace
model()outright. Not sure how he imagines it would interact withbeforeModel()andafterModel()or if these would even still exist in such an API.