From 3d09f869360db22801c64aeb1286a36d679d4331 Mon Sep 17 00:00:00 2001 From: Nick Iaconis Date: Thu, 1 Oct 2015 16:37:45 -0700 Subject: [PATCH 1/6] Create parallel model hook rfc --- text/0000-parallel-model-hooks.md | 115 ++++++++++++++++++++++++++++++ 1 file changed, 115 insertions(+) create mode 100644 text/0000-parallel-model-hooks.md diff --git a/text/0000-parallel-model-hooks.md b/text/0000-parallel-model-hooks.md new file mode 100644 index 0000000000..04faf5e939 --- /dev/null +++ b/text/0000-parallel-model-hooks.md @@ -0,0 +1,115 @@ +- Start Date: 2015-10-01 +- RFC PR: (leave this empty) +- Ember Issue: (leave this empty) + +# Summary + +Currently, Ember waits until a route's `beforeModel` hook resolves before triggering its `model` hook, waits until its `model` hook resolves before triggering its `afterModel` hook, and waits until its `afterModel` hook resolves before triggering a child route's `beforeModel` hook. +Performance wins can be achieved by running in parallel the model hooks for all routes related to a transition. + +# Motivation + +There are performance wins associated with running model hooks in parallel. +Child routes that do not depend on data from parent routes may make their requests at the same time as their parent routes. +This causes the child routes to resolve faster. +APIs that batch requests can experience additional performance gains because requests will batch across routes. +See this demo for a visualization: http://nickiaconis.github.io/ember-parallel-model-demo/ + +# Detailed design + +In short, we need to run all `beforeModel`, `model`, and `afterModel` hooks before resolving the promises they return. +This has two major implications: + +1. The `afterModel` hook will be passed a promise, since the result of the `model` hook will not have resolved by the time `afterModel` is called. +2. Routes may not have resolved the data requested by `modelFor` when it is called. It will need to return a promise or there will need to be another method for getting a promise that eventually resolves a route's model. (`modelPromiseFor`?) + +#### Router.js (tildeio/router.js#171) + +`handler-info` presently has a `resolve` method that calls the model hooks. We move the model hook calls to a new `request` method, and save the promises on the `handler-info`'s context (the promise from the `model` hook is passed to the `afterModel` hook). The `resolve` method takes the stored promises and inserts them into the resolution chain. + +`transition-state` presently has a `resolveOneHandlerInfo` method that recurses through the `handler-info`s for a transition and calls the associated `resolve` method. Before this, we insert a `requestOneHandlerInfo` method that recurses through `handler-info`s, calling each one's `request` method, which now executes the model hooks, kicking off any network requests. + +We add a `requestIndex` property to `transition` that keeps track of how many route's model hooks we have already run (akin to how `resolveIndex` presently keeps track of how many routes we have resolved). + +#### Ember.js (emberjs/ember.js#12415) + +The default model hook uses `requestIndex` (instead of `resolveIndex`) to grab `modelPromise` (instead of `context`) from the `handler-info`. + +`Route#modelFor` returns a promise. + +#### Usage + +Routes that don't depend on another route's data likely don't need any changes, but their requests are sent in parallel: + +```javascript +// app/routes/parent.js +export default Ember.Route.extend({ + model() { + return Ember.$.get('/api/foo'); + } +}); + +// app/routes/parent/child.js +export default Ember.Route.extend({ + model(params) { + return Ember.$.get(`/api/foo/${params.id}`); + } +}); +``` + +Routes that depend on another route's data must consume that data as a promise: + +```javascript +// app/routes/parent.js +export default Ember.Route.extend({ + model(params) { + return Ember.$.get(`/api/foo/${params.id}`); + } +}); + +// app/routes/parent/child.js +export default Ember.Route.extend({ + model() { + return this.modelFor('parent').then((parentModel) => { + return Ember.$.get(`/api/bar/${parentModel.get('baz')}`); + }); + } +}); +``` + +Routes whose `afterModel` hook operates on the resolved model must now consume a promise: + +```javascript +export default Ember.Route.extend({ + model(params) { + return Ember.$.get(`/api/foo/${params.id}`); + }, + afterModel(modelPromise, transition) { + return modelPromise.then((data) => { + if (data.abort) { + this.transitionTo('fiz'); + } + } + } +}); +``` + +# Drawbacks + +- This is a major breaking change. +- Having `Route#modelFor` return a promise may be confusing. + +# Alternatives + +- Tie this paradigm change to something opt-in like routable components where people will already have to change their behavior. +- Maintain it behind a feature flag on the Canary branch which you can toggle on and off via config until we can land it. +- Provide some built-in hooks so that we're able to reach in and change this behavior as an addon in a safer way. +- The worlds most absurd monkeypatch–which would also be packaged as an addon. +- Maintain a patch and build our own custom Ember. + +# Unresolved questions + + +- Should `modelFor` always return a promise? Should we instead add a `modelPromiseFor` method? + - If `modelFor` returns a promise, how do we make this an easy change for developers? + - If adding `modelPromiseFor`, what do calls to `modelFor` return before the model's promise has been fulfilled? From 8952f058efbb60f4c4488ffb95310428a2d51334 Mon Sep 17 00:00:00 2001 From: Nick Iaconis Date: Tue, 20 Oct 2015 16:27:57 -0700 Subject: [PATCH 2/6] Update and rename 0000-parallel-model-hooks.md to 0000-prefetch-hook.md --- text/0000-parallel-model-hooks.md | 115 ------------------------------ text/0000-prefetch-hook.md | 108 ++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+), 115 deletions(-) delete mode 100644 text/0000-parallel-model-hooks.md create mode 100644 text/0000-prefetch-hook.md diff --git a/text/0000-parallel-model-hooks.md b/text/0000-parallel-model-hooks.md deleted file mode 100644 index 04faf5e939..0000000000 --- a/text/0000-parallel-model-hooks.md +++ /dev/null @@ -1,115 +0,0 @@ -- Start Date: 2015-10-01 -- RFC PR: (leave this empty) -- Ember Issue: (leave this empty) - -# Summary - -Currently, Ember waits until a route's `beforeModel` hook resolves before triggering its `model` hook, waits until its `model` hook resolves before triggering its `afterModel` hook, and waits until its `afterModel` hook resolves before triggering a child route's `beforeModel` hook. -Performance wins can be achieved by running in parallel the model hooks for all routes related to a transition. - -# Motivation - -There are performance wins associated with running model hooks in parallel. -Child routes that do not depend on data from parent routes may make their requests at the same time as their parent routes. -This causes the child routes to resolve faster. -APIs that batch requests can experience additional performance gains because requests will batch across routes. -See this demo for a visualization: http://nickiaconis.github.io/ember-parallel-model-demo/ - -# Detailed design - -In short, we need to run all `beforeModel`, `model`, and `afterModel` hooks before resolving the promises they return. -This has two major implications: - -1. The `afterModel` hook will be passed a promise, since the result of the `model` hook will not have resolved by the time `afterModel` is called. -2. Routes may not have resolved the data requested by `modelFor` when it is called. It will need to return a promise or there will need to be another method for getting a promise that eventually resolves a route's model. (`modelPromiseFor`?) - -#### Router.js (tildeio/router.js#171) - -`handler-info` presently has a `resolve` method that calls the model hooks. We move the model hook calls to a new `request` method, and save the promises on the `handler-info`'s context (the promise from the `model` hook is passed to the `afterModel` hook). The `resolve` method takes the stored promises and inserts them into the resolution chain. - -`transition-state` presently has a `resolveOneHandlerInfo` method that recurses through the `handler-info`s for a transition and calls the associated `resolve` method. Before this, we insert a `requestOneHandlerInfo` method that recurses through `handler-info`s, calling each one's `request` method, which now executes the model hooks, kicking off any network requests. - -We add a `requestIndex` property to `transition` that keeps track of how many route's model hooks we have already run (akin to how `resolveIndex` presently keeps track of how many routes we have resolved). - -#### Ember.js (emberjs/ember.js#12415) - -The default model hook uses `requestIndex` (instead of `resolveIndex`) to grab `modelPromise` (instead of `context`) from the `handler-info`. - -`Route#modelFor` returns a promise. - -#### Usage - -Routes that don't depend on another route's data likely don't need any changes, but their requests are sent in parallel: - -```javascript -// app/routes/parent.js -export default Ember.Route.extend({ - model() { - return Ember.$.get('/api/foo'); - } -}); - -// app/routes/parent/child.js -export default Ember.Route.extend({ - model(params) { - return Ember.$.get(`/api/foo/${params.id}`); - } -}); -``` - -Routes that depend on another route's data must consume that data as a promise: - -```javascript -// app/routes/parent.js -export default Ember.Route.extend({ - model(params) { - return Ember.$.get(`/api/foo/${params.id}`); - } -}); - -// app/routes/parent/child.js -export default Ember.Route.extend({ - model() { - return this.modelFor('parent').then((parentModel) => { - return Ember.$.get(`/api/bar/${parentModel.get('baz')}`); - }); - } -}); -``` - -Routes whose `afterModel` hook operates on the resolved model must now consume a promise: - -```javascript -export default Ember.Route.extend({ - model(params) { - return Ember.$.get(`/api/foo/${params.id}`); - }, - afterModel(modelPromise, transition) { - return modelPromise.then((data) => { - if (data.abort) { - this.transitionTo('fiz'); - } - } - } -}); -``` - -# Drawbacks - -- This is a major breaking change. -- Having `Route#modelFor` return a promise may be confusing. - -# Alternatives - -- Tie this paradigm change to something opt-in like routable components where people will already have to change their behavior. -- Maintain it behind a feature flag on the Canary branch which you can toggle on and off via config until we can land it. -- Provide some built-in hooks so that we're able to reach in and change this behavior as an addon in a safer way. -- The worlds most absurd monkeypatch–which would also be packaged as an addon. -- Maintain a patch and build our own custom Ember. - -# Unresolved questions - - -- Should `modelFor` always return a promise? Should we instead add a `modelPromiseFor` method? - - If `modelFor` returns a promise, how do we make this an easy change for developers? - - If adding `modelPromiseFor`, what do calls to `modelFor` return before the model's promise has been fulfilled? diff --git a/text/0000-prefetch-hook.md b/text/0000-prefetch-hook.md new file mode 100644 index 0000000000..c12197298d --- /dev/null +++ b/text/0000-prefetch-hook.md @@ -0,0 +1,108 @@ +- Start Date: 2015-10-01 +- RFC PR: (leave this empty) +- Ember Issue: (leave this empty) + +# Summary + +The waterfall way in which Ember resolves a transition's model hooks can result in unnecessary blocking. +Performance wins can be achieved by allowing routes to decouple initiating network requests from waiting for their completion. + +# 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. +As such, a route that defines a `prefetch` hook is not required to define a `model` hook. + +A `prefetched` method is added to `Route`. +It is used to provide a route access to a parent route's prefetched data much the way `modelFor` provides access to a parent's model. +`prefetched` always returns a promise. + +```javascript +App.TwoBlueFishRoute = Ember.Route.extend({ + prefetch() { + return this.prefetched('one-red-fish') + .then((fish) => { + if (fish.get('isOne')) { + return Ember.$.get('/api/twoFish'); + } else { + return Ember.$.get('/api/blueFish'); + } + }); + } +}); +``` + +The syntax is simplified by ES7 async functions. + +```javascript +App.TwoBlueFishRoute = Ember.Route.extend({ + async prefetch() { + if ((await this.prefetched('one-red-fish')).get('isOne')) { + return Ember.$.get('/api/twoFish'); + } else { + return Ember.$.get('/api/blueFish'); + } + } +}); +``` + +`prefetched` can also be used to get a route's own data. +This is useful for expanding upon the prefetched data in the route's `model` hook. + +```javascript +App.PostCommentsRoute = Ember.Route.extend({ + prefetch(params, transition) { + return Ember.$.get(`/api/posts/${transition.params.post.id}/comments`); + }, + + async model() { + return { + OP: this.modelFor('post')).author, + comments: await this.prefetched(this.routeName) + }; + } +}); +``` + +# Drawbacks + +- Ember's API becomes larger. + +# Alternatives + +- Provide something like `Router#willTransition`, but that is triggered on redirects, so this can be built as an addon. It could be triggered on either all transitions (`Router#willChangeURL`?) or only redirects (`Router#willRedirect`? to supplement `willTransition`). + +# Unresolved questions + +- Should the new method be named `asyncModelFor` instead of `prefetched`? From 3f8ee4973c7bfae0696e891ba39a6ba7ec024255 Mon Sep 17 00:00:00 2001 From: Nick Iaconis Date: Wed, 4 Nov 2015 10:45:25 -0800 Subject: [PATCH 3/6] Update 0000-prefetch-hook.md --- text/0000-prefetch-hook.md | 41 ++++++-------------------------------- 1 file changed, 6 insertions(+), 35 deletions(-) diff --git a/text/0000-prefetch-hook.md b/text/0000-prefetch-hook.md index c12197298d..0306b38cb2 100644 --- a/text/0000-prefetch-hook.md +++ b/text/0000-prefetch-hook.md @@ -44,38 +44,9 @@ App.PostCommentsRoute = Ember.Route.extend({ The default functionality of the `model` hook is modified to return the prefetched data if it exists. As such, a route that defines a `prefetch` hook is not required to define a `model` hook. -A `prefetched` method is added to `Route`. -It is used to provide a route access to a parent route's prefetched data much the way `modelFor` provides access to a parent's model. -`prefetched` always returns a promise. - -```javascript -App.TwoBlueFishRoute = Ember.Route.extend({ - prefetch() { - return this.prefetched('one-red-fish') - .then((fish) => { - if (fish.get('isOne')) { - return Ember.$.get('/api/twoFish'); - } else { - return Ember.$.get('/api/blueFish'); - } - }); - } -}); -``` - -The syntax is simplified by ES7 async functions. - -```javascript -App.TwoBlueFishRoute = Ember.Route.extend({ - async prefetch() { - if ((await this.prefetched('one-red-fish')).get('isOne')) { - return Ember.$.get('/api/twoFish'); - } else { - return Ember.$.get('/api/blueFish'); - } - } -}); -``` +A `prefetched` property is added to `Route`. +It is used to pass a route's prefetched data from the `prefetch` hook to the default `model` hook. +`prefetched` will always be a promise. `prefetched` can also be used to get a route's own data. This is useful for expanding upon the prefetched data in the route's `model` hook. @@ -89,7 +60,7 @@ App.PostCommentsRoute = Ember.Route.extend({ async model() { return { OP: this.modelFor('post')).author, - comments: await this.prefetched(this.routeName) + comments: await this.prefetched }; } }); @@ -101,8 +72,8 @@ App.PostCommentsRoute = Ember.Route.extend({ # Alternatives -- Provide something like `Router#willTransition`, but that is triggered on redirects, so this can be built as an addon. It could be triggered on either all transitions (`Router#willChangeURL`?) or only redirects (`Router#willRedirect`? to supplement `willTransition`). +- Provide something like `Router#willTransition`, but that is triggered on redirects, so this can be built as an addon. It could be triggered on either all transitions (`Router#willChangeURL`?) or only redirects (`Router#willRedirect`? to supplement `willTransition`). Otherwise, building this as an addon must change the functionality of `Router#willTransition` in order to function properly. # Unresolved questions -- Should the new method be named `asyncModelFor` instead of `prefetched`? +- Is there a use case for the API to include a method like `modelFor`, but for `prefetch`? (`prefetchedFor`/`asyncModelFor`) From fae441c0bd653d594a8c0968a1b678e350f8fcaf Mon Sep 17 00:00:00 2001 From: Nick Iaconis Date: Thu, 19 Nov 2015 19:40:04 -0800 Subject: [PATCH 4/6] Update 0000-prefetch-hook.md --- text/0000-prefetch-hook.md | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/text/0000-prefetch-hook.md b/text/0000-prefetch-hook.md index 0306b38cb2..6fe540a40e 100644 --- a/text/0000-prefetch-hook.md +++ b/text/0000-prefetch-hook.md @@ -4,8 +4,14 @@ # Summary -The waterfall way in which Ember resolves a transition's model hooks can result in unnecessary blocking. -Performance wins can be achieved by allowing routes to decouple initiating network requests from waiting for their completion. +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 @@ -56,7 +62,7 @@ App.PostCommentsRoute = Ember.Route.extend({ prefetch(params, transition) { return Ember.$.get(`/api/posts/${transition.params.post.id}/comments`); }, - + async model() { return { OP: this.modelFor('post')).author, From b6b261cfb28902fc3b114db4ea3925a93979b46d Mon Sep 17 00:00:00 2001 From: Nick Iaconis Date: Thu, 4 Feb 2016 16:21:50 -0800 Subject: [PATCH 5/6] Update 0000-prefetch-hook.md Change prefetched to a method and update info about addon alternative. --- text/0000-prefetch-hook.md | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/text/0000-prefetch-hook.md b/text/0000-prefetch-hook.md index 6fe540a40e..b98b39ad88 100644 --- a/text/0000-prefetch-hook.md +++ b/text/0000-prefetch-hook.md @@ -50,12 +50,10 @@ App.PostCommentsRoute = Ember.Route.extend({ The default functionality of the `model` hook is modified to return the prefetched data if it exists. As such, a route that defines a `prefetch` hook is not required to define a `model` hook. -A `prefetched` property is added to `Route`. -It is used to pass a route's prefetched data from the `prefetch` hook to the default `model` hook. -`prefetched` will always be a promise. - -`prefetched` can also be used to get a route's own data. -This is useful for expanding upon the prefetched data in the route's `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({ @@ -66,7 +64,7 @@ App.PostCommentsRoute = Ember.Route.extend({ async model() { return { OP: this.modelFor('post')).author, - comments: await this.prefetched + comments: await this.prefetched() }; } }); @@ -78,8 +76,7 @@ App.PostCommentsRoute = Ember.Route.extend({ # Alternatives -- Provide something like `Router#willTransition`, but that is triggered on redirects, so this can be built as an addon. It could be triggered on either all transitions (`Router#willChangeURL`?) or only redirects (`Router#willRedirect`? to supplement `willTransition`). Otherwise, building this as an addon must change the functionality of `Router#willTransition` in order to function properly. - -# Unresolved questions - -- Is there a use case for the API to include a method like `modelFor`, but for `prefetch`? (`prefetchedFor`/`asyncModelFor`) +- 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#willRedirect`? to supplement `willTransition`). + - 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`. From 79078ba3ecf9e21cb62d4a2c7ce20274658f44f9 Mon Sep 17 00:00:00 2001 From: Nick Iaconis Date: Thu, 4 Feb 2016 16:23:59 -0800 Subject: [PATCH 6/6] Update 0000-prefetch-hook.md --- text/0000-prefetch-hook.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-prefetch-hook.md b/text/0000-prefetch-hook.md index b98b39ad88..9b35a85680 100644 --- a/text/0000-prefetch-hook.md +++ b/text/0000-prefetch-hook.md @@ -77,6 +77,6 @@ App.PostCommentsRoute = Ember.Route.extend({ # 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#willRedirect`? to supplement `willTransition`). + - 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`). - 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`.