From df29bdc7a6af9416ce9fbba27c6548e2d0c6ac55 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Thu, 24 Sep 2015 18:34:11 -0400 Subject: [PATCH 01/16] Create Router Service --- text/0000-router-service.md | 252 ++++++++++++++++++++++++++++++++++++ 1 file changed, 252 insertions(+) create mode 100644 text/0000-router-service.md diff --git a/text/0000-router-service.md b/text/0000-router-service.md new file mode 100644 index 0000000000..6c8ef889d8 --- /dev/null +++ b/text/0000-router-service.md @@ -0,0 +1,252 @@ +- Start Date: 2015-09-24 +- RFC PR: +- Ember Issue: + +# Summary + +This RFC proposes: + + - creating a public `router` service that is a superset of today's `Ember.Router`. + + - codifying and expanding the supported public API for the `transition` object that is currently passed to `Route` hooks. + +These topics are closely related because they share a unified `RouteInfo` type, which will be described in detail. + +# Motivation + +Given the modern Ember concepts of Components and Services, it is clear that routing capability should be exposed as a Service. I hope this is uncontroversial, given that we already implement it as a service internally, and given that usage of these nominally-private APIs is already becoming widespread. + +The immediate benefit of having a `RouterService` is that you can inject it into components, giving them a friendly way to initiate transitions and ask questions about the current global router state. + +A second benefit is that we have the opportunity to add new capabilities to the `RouterService` to replace several common patterns in the wild that dive into private internals in order to get things done. There are several places where we leak internals from router.js, and we can plug those leaks. + +# Detailed design + +## RouterService + +By way of a simple example, the router service behaves like this: + +```js +import Component from 'ember-component'; +import service from 'ember-service/inject'; + +export default Component.extend({ + router: service(), + actions: { + goToMars() { + this.get('router').transitionTo('planet.mars'); + } + } +}); +``` + +Like any Service, it can also be injected into Helpers, Routes, etc. + +### Relationship between EmberRouter and RouterService + +Q: "Why are you calling this thing 'router' when we already have a router? Shouldn't the new thing be called 'routing' or something else?". + +A: We shouldn't have two things. From the user's perspective, there is just "the router", and it happens to be available as a service. While we're free to continue implementing it as multiple classes under the hood, the public API should present as a single, coherent concept. + +Terminology: + + - `EmberRouter` is the class that we already have today, defined in `ember-routing/system/router` and available publicly as `Ember.Router` + - `RouterService` is the new class I am proposing. + +`EmberRouter` has the following public API today: + + - `map` + - `location` + - `rootURL` + - `willTransition` + - `didTransition` + +That API will be carried over verbatim to `RouterService`, and the publicly accessible `Ember.Router` class will *become* `RouterService`. In terms of implementation, I expect the existing `EmberRouter` class will continue to exist mostly unchanged. But public access to it will be moderated through `RouterService`. + +### New Methods: Initiating Transitions + +```js +transitionTo(routeName, ...models, queryParams) +replaceWith(routeName, ...models, queryParms) +``` + +These two have the same semantics as the existing methods on `Ember.Route`: + +### New Method: Checking For Active Route + + - `isActive(routeName, ...models, queryParams)` + +The arguments have the same semantics as `transitionTo`, the return value is a boolean. This should provide the same logic that determines whether to put an active class on a `link-to`. Here's an example of how we can implement `is-active` as a helper, using this method: + +```js +import Helper from 'ember-helper'; +import service from 'ember-service/inject'; +import observer from 'ember-metal/observer'; + +export default Helper.extend({ + router: service(), + compute([routeName, ...models], hash) { + let allModels; + if (hash.models) { + allModels = models.concat(hash.models); + } else { + allModels = models; + } + return this.get('router').isActive(routeName, ...models, hash.queryParams); + }, + observer('router.currentRoute', function() { + this.recompute(); + }); +}); +``` + +```hbs +{{!- Example usage -}} +
  • + +{{!- Example usage with generic routeName and list of models (avoids splat) -}} + + +{{!- Note that the complexities of currentWhen can be avoided by composing instead. }} + + +``` + + +### New Method: URL generation + +`url(routeName, ...models, queryParams)` + +This takes the same arguments as `transitionTo`, but instead of initiating the transition it returns the resulting URL as a string. + +A `url-for` helper can be implemented almost identically to the `is-active` example above. + + +### New Properties + +`currentRoute`: an observable property. It is guaranteed to change whenever a route transition happens (even when that transition only changes parameters and doesn't change the active route). You should consider its value deeply immutable -- we will replace the whole structure whenever it changes. The value of `currentRoute` is a `RouteInfo` representing the current leaf route. `RouteInfo` is described below. + +`currentRouteName`: a convenient alias for `currentRoute.name`. + +`currentURL`: provides the serialized string representing `currentRoute`. + + +### Deprecation + +I propose deprecating the publicly extensible `willTransition` and `didTransition` hooks. They are redundant with an observable `currentRoute`, and the arguments they receive leak internal implemetation from router.js. + +## RouteInfo Type + +A RouteInfo object has the following properties. They are all read-only. + + - name: the dot-separated, fully-qualified name of this route, like `"people.index"`. + - localName: the final part of the `name`, like `"index"`. + - attrs: the attributes provided to this route's component, if any, like `{ model }`. For old-style routes that have a controller/view/template instead of a routable component, the implicit `attrs` shall contain `{ model, controller, view }`. + - params: the values of this route's parameters. Same as the argument to `Route`'s `model` hook. Contains only the parameters valid for this route, if any (params for parent or child routes are not merged). + - queryParams: the values of any queryParams on this route. + - parent: another RouteInfo instance, describing this route's parent route, if any. + - child: another RouteInfo instance, describing this route's active child route, if any. + +Notice that the `parent` and `child` properties cause `RouteInfos` to form a linked list. So even though the `currentRoute` property on `RouterService` points at the leafmost route, it can be traversed to discover everything about all active routes. As a convenience, `RouteInfo` also implements `Enumerable` over all the reachable `RouteInfos` from topmost to leafmost. This makes it possible to say things like: + +```js +router.currentRoute.find(info => info.name === 'people').params +``` + +## Transition Object + +A `transition` argument is passed to `Route`'s `beforeModel`, `model`, `afterModel`, and `willTransition` hooks. Today it's public API is only really `abort()` and `retry()`. + +### New Properties: `from` and `to` + +I'm proposing we add `from` and `to` properties on `transition` whose values are `RouteInfo` instances representing the initial and final leafmost routes for this transition. Like all RouteInfos, these are read-only and internally immutable. They are not observable, because a `transition` instance is never changed after creation. + +On an initial full-page load, the `from` property will be `null`. This creates a public API for distinguishing in-app transitions from full-page reloads. + +### Example: testing whether route will remain active + +Here's an example showing how `willTransition` can figure out if the current route will remain active after the transition: + +```js +willTransition(transition) { + if (!this.transition.to.find(route => route.name === this.routeName)) { + alert("Please save or cancel your changes."); + transition.abort(); + } +} +``` + +### Example: parent redirecting to a fallback model + +Here's an example of a parent route that can redirect to a fallback model, without losing its child route: + +```js +this.route('person', { path: '/person/:person_id' }, function() { + this.route('index'); + this.route('detail'); +}); + +//PersonRoute +const fallbackPersonId = 0; +model({ personId }, transition) { + return this.get('store').find('person', personId).catch(err => { + this.replaceWith(transition.to.name, fallbackPersonId); + }); +} + +// If personId 5 is invalid, and the user visits /person/5/detail, they will get +// redirected to /person/0/detail. And /person/5 will get redirected to /person/0. +``` + + +### Actively discourage use of private API + +This RFC provides public API for doing the things people have become accustomed to doing via private properties on `transition`. To eliminate confusion over the correct way, we should hide all the private API away behind symbols, and provide deprecation warnings per our usual release policy around breaking "widely-used private APIs" + + +# Drawbacks + +This RFC suggests only two small deprecations that are unlikely to effect many apps, so the API-churn burden may appear low. However, we know that use of the private APIs we're deliberately disabling is widespread, so users will experience churn. We can provide our usual deprecation cycle to give them early warning, but it still imposes some cost. + +This RFC doesn't attempt to change the existing and fairly rich semantics for initiating transitions. For example, you can pass either models or IDs, and those have subtle semantic differences. I think an ideal rewrite would also change the semantics of the route hooks and transitionTo to simplify that area. + +# Alternatives + +## Less Churn + +We could adopt some of the existing broadly used APIs as de-facto public. This avoids churn, but imposes a complexity burden on every new learner, who needs to be told "this is a weird API, but it's what we're stuck with". + +## Semver Lawyering + +I'm interepreting router.js's public/private documentation as out-of-scope for Ember's semver. The fact that we pass an instance of router.js's Transition as our `transition` argument is not documented. An alternative interpretation is that we need to continue supporting those methods marked as public in router.js's docs. + +## Optional Helpers + +I didn't propose shipping `is-active` and `url-for` template helpers -- I merely showed that they're easy to build using the router service. But we should arguably just ship them as part of the framework too. + +## Branching Route Hierarchies + +I am implicitly assuming we will only ever have linear route hierarchies, where a given route has at most one child. I can imagine eventually wanting a way to support branching route hierarchies, where each branch can transition independently. I'm not trying to account for that future. + +## Route.parentRoute + +This RFC makes it possible for a route to determine its parent's name dynamically via public API, and thus access its parent's model/params/controller: + +```js +beforeModel(transition) { + const parentInfo = transition.to.find(info => info.name === this.routeName).parent; + const parentModel = this.modelFor(parentInfo.name); +} +``` + +However, this pattern feels awkward, and I think it justifies just adding a public `parentRouteName()` method to `Route` that would simplify to: + +```js +beforeModel(transition) { + const parentModel = this.modelFor(this.parentRouteName()); +} +``` +Possibly we *want* this to feel awkward because it's a weird thing to do. + + + From edc37233edfb75b3989bdb6528cb4921ad40d5cd Mon Sep 17 00:00:00 2001 From: Ricardo Mendes Date: Mon, 30 May 2016 16:41:53 +0100 Subject: [PATCH 02/16] typo --- text/0000-router-service.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-router-service.md b/text/0000-router-service.md index 6c8ef889d8..c1e94b85f8 100644 --- a/text/0000-router-service.md +++ b/text/0000-router-service.md @@ -218,7 +218,7 @@ We could adopt some of the existing broadly used APIs as de-facto public. This a ## Semver Lawyering -I'm interepreting router.js's public/private documentation as out-of-scope for Ember's semver. The fact that we pass an instance of router.js's Transition as our `transition` argument is not documented. An alternative interpretation is that we need to continue supporting those methods marked as public in router.js's docs. +I'm interpreting router.js's public/private documentation as out-of-scope for Ember's semver. The fact that we pass an instance of router.js's Transition as our `transition` argument is not documented. An alternative interpretation is that we need to continue supporting those methods marked as public in router.js's docs. ## Optional Helpers From fc06b8be2aefa78e0ac3567f4d8f9911acffbb0b Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Sun, 12 Jun 2016 15:28:55 -0700 Subject: [PATCH 03/16] typo fix --- text/0000-router-service.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-router-service.md b/text/0000-router-service.md index c1e94b85f8..d9c2a02bdc 100644 --- a/text/0000-router-service.md +++ b/text/0000-router-service.md @@ -94,7 +94,7 @@ export default Helper.extend({ } return this.get('router').isActive(routeName, ...models, hash.queryParams); }, - observer('router.currentRoute', function() { + didTransition: observer('router.currentRoute', function() { this.recompute(); }); }); From 489fbeeeec76981f5759e2f414b555b3898f010a Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Sun, 12 Jun 2016 15:41:47 -0700 Subject: [PATCH 04/16] remove mention of `view` because that's already gone --- text/0000-router-service.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-router-service.md b/text/0000-router-service.md index d9c2a02bdc..50b5327ddb 100644 --- a/text/0000-router-service.md +++ b/text/0000-router-service.md @@ -141,7 +141,7 @@ A RouteInfo object has the following properties. They are all read-only. - name: the dot-separated, fully-qualified name of this route, like `"people.index"`. - localName: the final part of the `name`, like `"index"`. - - attrs: the attributes provided to this route's component, if any, like `{ model }`. For old-style routes that have a controller/view/template instead of a routable component, the implicit `attrs` shall contain `{ model, controller, view }`. + - attrs: the values that were resolved by this `Route`. Today, that means `{ model, controller }`. In the future it may become the attributes passed directly to a "routeable component". - params: the values of this route's parameters. Same as the argument to `Route`'s `model` hook. Contains only the parameters valid for this route, if any (params for parent or child routes are not merged). - queryParams: the values of any queryParams on this route. - parent: another RouteInfo instance, describing this route's parent route, if any. From dc7e38de913b603914a9db45d02973ff4d8d621e Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Sun, 12 Jun 2016 16:56:48 -0700 Subject: [PATCH 05/16] Don't deprecate Router.willTransition and didTransition --- text/0000-router-service.md | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/text/0000-router-service.md b/text/0000-router-service.md index 50b5327ddb..dd39cccb39 100644 --- a/text/0000-router-service.md +++ b/text/0000-router-service.md @@ -131,10 +131,6 @@ A `url-for` helper can be implemented almost identically to the `is-active` exam `currentURL`: provides the serialized string representing `currentRoute`. -### Deprecation - -I propose deprecating the publicly extensible `willTransition` and `didTransition` hooks. They are redundant with an observable `currentRoute`, and the arguments they receive leak internal implemetation from router.js. - ## RouteInfo Type A RouteInfo object has the following properties. They are all read-only. @@ -155,7 +151,7 @@ router.currentRoute.find(info => info.name === 'people').params ## Transition Object -A `transition` argument is passed to `Route`'s `beforeModel`, `model`, `afterModel`, and `willTransition` hooks. Today it's public API is only really `abort()` and `retry()`. +A `transition` argument is passed to `Route#beforeModel`, `Route#model`, `Route#afterModel`, `Route#willTransition`, and `Router#willTransition`. Today `transition`'s public API is only really `abort()` and `retry()`. ### New Properties: `from` and `to` @@ -206,7 +202,7 @@ This RFC provides public API for doing the things people have become accustomed # Drawbacks -This RFC suggests only two small deprecations that are unlikely to effect many apps, so the API-churn burden may appear low. However, we know that use of the private APIs we're deliberately disabling is widespread, so users will experience churn. We can provide our usual deprecation cycle to give them early warning, but it still imposes some cost. +This RFC doesn't deprecate any public API, so the API-churn burden may appear low. However, we know that use of the private APIs we're deliberately disabling is widespread, so users will experience churn. We can provide our usual deprecation cycle to give them early warning, but it still imposes some cost. This RFC doesn't attempt to change the existing and fairly rich semantics for initiating transitions. For example, you can pass either models or IDs, and those have subtle semantic differences. I think an ideal rewrite would also change the semantics of the route hooks and transitionTo to simplify that area. From ca250a0271f685d122ffc3bfaf394a1afab2cd6a Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Sun, 12 Jun 2016 17:00:33 -0700 Subject: [PATCH 06/16] Adding `recognize` to close another public API gap --- text/0000-router-service.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/text/0000-router-service.md b/text/0000-router-service.md index dd39cccb39..54b342b4d5 100644 --- a/text/0000-router-service.md +++ b/text/0000-router-service.md @@ -121,6 +121,13 @@ This takes the same arguments as `transitionTo`, but instead of initiating the t A `url-for` helper can be implemented almost identically to the `is-active` example above. +### New Method: URL recognition + +`recognize(url)` + +Takes a string URL (relative to the application's baseURL) and returns a `RouteInfo` for the leafmost route represented by the URL. Returns `null` if the URL is not recognized. + +Example: this feature can replace [this use of private API in ember-href-to](https://github.com/intercom/ember-href-to/blob/b8cf0699eec6a65570b05e4fc22b27d8cea49c42/app/instance-initializers/browser/ember-href-to.js#L34). ### New Properties From 7e29f4acef91ba81e4c511e63fcc467d1f1a7118 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Sun, 12 Jun 2016 17:07:39 -0700 Subject: [PATCH 07/16] Dropping `attrs` property This was a nice-to-have, but unlike the rest of the properties here it's not synchronously available. `modelFor`/`controllerFor` already provide a good way to get these things when needed. --- text/0000-router-service.md | 1 - 1 file changed, 1 deletion(-) diff --git a/text/0000-router-service.md b/text/0000-router-service.md index 54b342b4d5..de92545228 100644 --- a/text/0000-router-service.md +++ b/text/0000-router-service.md @@ -144,7 +144,6 @@ A RouteInfo object has the following properties. They are all read-only. - name: the dot-separated, fully-qualified name of this route, like `"people.index"`. - localName: the final part of the `name`, like `"index"`. - - attrs: the values that were resolved by this `Route`. Today, that means `{ model, controller }`. In the future it may become the attributes passed directly to a "routeable component". - params: the values of this route's parameters. Same as the argument to `Route`'s `model` hook. Contains only the parameters valid for this route, if any (params for parent or child routes are not merged). - queryParams: the values of any queryParams on this route. - parent: another RouteInfo instance, describing this route's parent route, if any. From eee39cc2904b7ee644912cf5cf7157a9c4350de0 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Sun, 12 Jun 2016 17:30:30 -0700 Subject: [PATCH 08/16] elaborate on some private APIs to deprecate --- text/0000-router-service.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/text/0000-router-service.md b/text/0000-router-service.md index de92545228..27d33be9aa 100644 --- a/text/0000-router-service.md +++ b/text/0000-router-service.md @@ -203,7 +203,13 @@ model({ personId }, transition) { ### Actively discourage use of private API -This RFC provides public API for doing the things people have become accustomed to doing via private properties on `transition`. To eliminate confusion over the correct way, we should hide all the private API away behind symbols, and provide deprecation warnings per our usual release policy around breaking "widely-used private APIs" +This RFC provides public API for doing the things people have become accustomed to doing via private API. To eliminate confusion over the correct way, we should hide all the private API away behind symbols, and provide deprecation warnings per our usual release policy around breaking "widely-used private APIs". + +Some of the private APIs we should mark and warn include: + + - transition.state + - transition.params + - `lookup('router:main')` (should use `service:router` instead) # Drawbacks From 84f5239100d2c6d20cb74382ba7eedfc5fa3b8b4 Mon Sep 17 00:00:00 2001 From: Ricardo Mendes Date: Wed, 3 Aug 2016 00:26:17 +0100 Subject: [PATCH 09/16] Update 0000-router-service.md --- text/0000-router-service.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-router-service.md b/text/0000-router-service.md index 27d33be9aa..21f8f6e859 100644 --- a/text/0000-router-service.md +++ b/text/0000-router-service.md @@ -115,7 +115,7 @@ export default Helper.extend({ ### New Method: URL generation -`url(routeName, ...models, queryParams)` +`urlFor(routeName, ...models, queryParams)` This takes the same arguments as `transitionTo`, but instead of initiating the transition it returns the resulting URL as a string. From e3bfde632362e2e9dd00458d1d65fea80a4a9134 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Tue, 23 Aug 2016 01:05:55 -0400 Subject: [PATCH 10/16] updates --- text/0000-router-service.md | 153 +++++++++++++++++++++++++++++++++++- 1 file changed, 149 insertions(+), 4 deletions(-) diff --git a/text/0000-router-service.md b/text/0000-router-service.md index 21f8f6e859..e156f394ad 100644 --- a/text/0000-router-service.md +++ b/text/0000-router-service.md @@ -10,6 +10,8 @@ This RFC proposes: - codifying and expanding the supported public API for the `transition` object that is currently passed to `Route` hooks. + - introducing the concept of dynamically-scoped routing info. + These topics are closely related because they share a unified `RouteInfo` type, which will be described in detail. # Motivation @@ -20,6 +22,16 @@ The immediate benefit of having a `RouterService` is that you can inject it into A second benefit is that we have the opportunity to add new capabilities to the `RouterService` to replace several common patterns in the wild that dive into private internals in order to get things done. There are several places where we leak internals from router.js, and we can plug those leaks. +A `RouterService` is great for asking global questions, but some questions are not global and today we incur complexity by treating them as if they are. For example: + + - `{{link-to}}` can use implicit models from its context, but that breaks when you're trying to animate to or from a state where those models are not present. + + - `{{link-to}}` has a lot of complexity and performance cost that deals with changing its active state, and the precise timing of when that should happen. + + - there is no way to ask the router what it would do to handle a given URL without actually visting that URL. + +All of the above can be addressed by embracing what is already internally true: "the current route" is not a single global, it's a dynamically-scoped variable that can have different values in different parts of the application simultaneously. + # Detailed design ## RouterService @@ -67,7 +79,7 @@ That API will be carried over verbatim to `RouterService`, and the publicly acce ```js transitionTo(routeName, ...models, queryParams) -replaceWith(routeName, ...models, queryParms) +replaceWith(routeName, ...models, queryParams) ``` These two have the same semantics as the existing methods on `Ember.Route`: @@ -112,12 +124,11 @@ export default Helper.extend({ ``` - ### New Method: URL generation `urlFor(routeName, ...models, queryParams)` -This takes the same arguments as `transitionTo`, but instead of initiating the transition it returns the resulting URL as a string. +This takes the same arguments as `transitionTo`, but instead of initiating the transition it returns the resulting root-relative URL as a string (which will include the application's `rootUrl`). A `url-for` helper can be implemented almost identically to the `is-active` example above. @@ -125,10 +136,24 @@ A `url-for` helper can be implemented almost identically to the `is-active` exam `recognize(url)` -Takes a string URL (relative to the application's baseURL) and returns a `RouteInfo` for the leafmost route represented by the URL. Returns `null` if the URL is not recognized. +Takes a string URL and returns a `RouteInfo` for the leafmost route represented by the URL. Returns `null` if the URL is not recognized. This method expects to receive the actual URL as seen by the browser _including_ the app's `rootURL`. + Example: this feature can replace [this use of private API in ember-href-to](https://github.com/intercom/ember-href-to/blob/b8cf0699eec6a65570b05e4fc22b27d8cea49c42/app/instance-initializers/browser/ember-href-to.js#L34). + +### New Method: Recognize and load models + +`recognizeAndLoad(url)` + +Takes a string URL and returns a promise that resolves to a `RouteInfoWithAttributes` for the leafmost route represented by the URL. The promise rejects if the URL is not recognized or an unhandled exception is encountered. This method expects to receive the actual URL as seen by the browser _including_ the app's `rootURL`. + +### Improved Event Coverage + +Application-wide transition monitoring events belong on the Router service, not spread throughout the Route classes. That is the reason for the existing `willTransition` and `didTransition` hooks/events on the Router. But they are not sufficient to capture all the detail people need. See for example, https://github.com/nickiaconis/rfcs/blob/1bd98ec534441a38f62a48599ffa8a63551b785f/text/0000-transition-hooks-events.md + +I do not believe an additional event is needed, we just need to ensure that the existing two events fire at every appropriate time. `willTransition` should fire _whenever_ the destination route changes, and that includes redirects, error handling, and loading states. A long chain of redirects should cause a sequence of `willTransition` events and then a final `didTransition` event. + ### New Properties `currentRoute`: an observable property. It is guaranteed to change whenever a route transition happens (even when that transition only changes parameters and doesn't change the active route). You should consider its value deeply immutable -- we will replace the whole structure whenever it changes. The value of `currentRoute` is a `RouteInfo` representing the current leaf route. `RouteInfo` is described below. @@ -137,6 +162,19 @@ Example: this feature can replace [this use of private API in ember-href-to](htt `currentURL`: provides the serialized string representing `currentRoute`. +### Query Parameter Semantics + +Today, `queryParams` impose unnecessarily high cost because we cannot generate URLs or determine if a link is active without taking into account the default values of query parameters. Determining their default values is expensive, because it involves instantiating the corresponding controller, even in cases where we will never visit its route. + +Therefore, the `queryParams` argument to the new `urlFor`, `transitionTo`, `replaceWith`, and `isActive` methods defined in this document will behave differently. + + - default values will not be stripped from generated URLs. For example, `urlFor('my-route', { sortBy: 'title' })` will always include `?sortBy=title`, whether or not `title` is the default value of `sortBy`. + + - to explicitly unset a query parameter, you can pass the symbol `Ember.DEFAULT_VALUE` as its value. For example, `transitionTo('my-route', { sortBy: Ember.DEFAULT_VALUE })` will result in a URL that does not contain any `?sortBy=`. + +(Sticky parameters are still allowed, because they only apply when the destination controller has already been instantiated anyway.) + + ## RouteInfo Type @@ -155,6 +193,10 @@ Notice that the `parent` and `child` properties cause `RouteInfos` to form a lin router.currentRoute.find(info => info.name === 'people').params ``` +## RouteInfoWithAttributes + +This type is almost identical to `RouteInfo`, except it has one additional proprty named `attributes`. The attributes contain the data that was loaded for this route, which is typically just `{ model }`. + ## Transition Object A `transition` argument is passed to `Route#beforeModel`, `Route#model`, `Route#afterModel`, `Route#willTransition`, and `Router#willTransition`. Today `transition`'s public API is only really `abort()` and `retry()`. @@ -212,6 +254,98 @@ Some of the private APIs we should mark and warn include: - `lookup('router:main')` (should use `service:router` instead) +## Dynamically-Scoped Variables + +The next section introduces general-purpose, dynamically-scoped variables. Why am I doing this in "The Router Service RFC"? Because dynamically-scoped RouteInfo has several nice capabilities that will be introduced in the subsequent section. And once we have one dynamically-scoped variable, we have the option of exposing that capability for other uses. + +### In the general case + +A [dynamically-scoped variable](https://en.wikipedia.org/wiki/Scope_(computer_science)#Dynamic_scoping) takes its value from its calling context, as opposed to its lexical context. Since dynamically-scoped variables are powerful to the point of potential danger, their syntax is intended to be appropriately verbose. + +Reading a dynamically-scoped variable in handlebars: + + ```hbs + {{!- retrieve the value of a dynamically scoped variable }} + (get-dynamic-variable "myVariableName") + ``` + +Defining a component that reads a dynamically-scoped variable: + +```js +let MyComponent = Ember.Component.extend({ + didInsertElement() { + // Access to the variable here is intended to be indistinguishable + // from a normal, explicitly-passed input argument. + doSomethingWith(this.get('myDynamicVariable')); + } +}); +MyComponent.reopenClass({ + // This is where the specialness happens. + getDynamicVariables: [ 'myDynamicVariable' ] +}); +``` + +Defining a helper that reads a dynamically-scoped variable: + +```js +let MyHelper = Ember.Helper.extend({ + compute(params, hash) { + return doSomethingWith(hash.myDynamicVariable); + } +}); +MyHelper.reopenClass({ + getDynamicVariables: [ 'myDynamicVariable' ] +}); +``` + +Setting a dynamically-scoped variable: + +```hbs +{{#with-dynamic-variable "myVariableName" someValue}} + {{!- + within this block AND ALL ITS DESCENDANTS until + otherwise overridden by another with-dynamic-variable statement, + `get-dynamic-variable "myVariableName"` returns someValue. + -}} +{{/with-dynamic-variable}} +``` + +Note that there is no `set-dynamic-variable`. You can only introduce new scopes, not mutate your containing scope. There is also no way to set a dynamically-scoped variable directly from Javascript -- your component must use a `with-dynamic-variable` block within its handlebars template. + +User-defined, dynamically-scoped variables must be pre-declared before first render via `Ember.declareDynamicVariable("myVariableName")`. Attempting to set one that wasn't declared is an error. Attempting to read one that doesn't exist returns `undefined`. Predeclaration is an optimization that allows us to maintain consistent performance within Glimmer. + +### The specific case of routeInfo + +`routeInfo` is a dynamically-scoped variable provided by Ember as public API. Its value is always a `RouteInfoWithAttributes` object that is correct for the given route context. This enables several nice things, which I will illustrate with examples: + +1. Here is a simplified `is-active` helper that will always update at the appropriate time to match exactly what is rendered in the current outlet. It will maintain the correct state even during animations. Instead of injecting the router service, it consumes the `routeInfo` from its containing environment: + +```js +Ember.Helper.extend({ + compute([routeName], { routeInfo }) { + return !!routeInfo.find(info => info.name === routeName); + } +}).reopenClass({ + getDynamicVariables: ['routeInfo'] +}); +``` + +A more complete version that also matches models and queryParams can be written in the same way. + +2. We can improve `link-to` so that it always finds implicit model arguments from the local context, rather than trying to locate them on the global router service. This will fix longstanding bugs like https://github.com/ember-animation/liquid-fire/issues/347 and it will make it easier to test components that contain `{{link-to}}`. This would also open the door to relative link-tos. + +3. `liquid-outlet` can be implemented entirely via public API. It would become: + +```hbs +{{#liquid-bind (get-dynamic-variable "routeInfo") as |currentRouteInfo|}} + {{#with-dynamic-variable "routeInfo" currentRouteInfo}} + {{outlet}} + {{/with-dynamic-variable}} +{{/liquid-bind}} + +4. Prerendering of non-current routes becomes possible. You can use `recognizeAndLoad` to obtain a `RouteInfoWithAttributes` and then use `{{#with-dynamic-variable "routeInfo" myRouteInfo}} {{outlet}} {{/with-dynamic-variable}}` to render it. + + # Drawbacks This RFC doesn't deprecate any public API, so the API-churn burden may appear low. However, we know that use of the private APIs we're deliberately disabling is widespread, so users will experience churn. We can provide our usual deprecation cycle to give them early warning, but it still imposes some cost. @@ -256,5 +390,16 @@ beforeModel(transition) { ``` Possibly we *want* this to feel awkward because it's a weird thing to do. +## Naming of Ember.DEFAULT_VALUE Symbol + +Should we introduce new API via the `Ember` global and switch to a module export once all the rest of Ember does, or should we just start with a module export right now? If so, what module? + + import { DEFAULT_VALUE } from 'ember-routing'; + +## Generic dynamically-scoped variables may be too powerful + +They let you do spooky-action-at-a-distance stuff. We could instad choose to implement single-purpose methods like `(get-route-info)` and `{{#with-route-info someValue}}` that would solve the routing case without opening the door to arbitrary user-defined ones. +## Possible semantic breakage of willTransition +I proposed fixing willTransition to ensure that it fires during redirects. I see that as a clear bugfix, but if there is reason to believe it will break people's apps then we should deprecate `willTransition` and introduce a new event with the correct semantics. From 01ad76a95bfee3452a99712dc60fad310f9b9f19 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Tue, 23 Aug 2016 01:31:47 -0400 Subject: [PATCH 11/16] update --- text/0000-router-service.md | 46 ++++++++++++++++++++++++++++++++----- 1 file changed, 40 insertions(+), 6 deletions(-) diff --git a/text/0000-router-service.md b/text/0000-router-service.md index e156f394ad..81805b8c37 100644 --- a/text/0000-router-service.md +++ b/text/0000-router-service.md @@ -148,11 +148,48 @@ Example: this feature can replace [this use of private API in ember-href-to](htt Takes a string URL and returns a promise that resolves to a `RouteInfoWithAttributes` for the leafmost route represented by the URL. The promise rejects if the URL is not recognized or an unhandled exception is encountered. This method expects to receive the actual URL as seen by the browser _including_ the app's `rootURL`. -### Improved Event Coverage +### Deprecating willTransition and didTransition Application-wide transition monitoring events belong on the Router service, not spread throughout the Route classes. That is the reason for the existing `willTransition` and `didTransition` hooks/events on the Router. But they are not sufficient to capture all the detail people need. See for example, https://github.com/nickiaconis/rfcs/blob/1bd98ec534441a38f62a48599ffa8a63551b785f/text/0000-transition-hooks-events.md -I do not believe an additional event is needed, we just need to ensure that the existing two events fire at every appropriate time. `willTransition` should fire _whenever_ the destination route changes, and that includes redirects, error handling, and loading states. A long chain of redirects should cause a sequence of `willTransition` events and then a final `didTransition` event. +In addition, they receive handlerInfos in their arguments, which are an undocumented internal implementation detail of router.js that doesn't belong in Ember's public API. Everything you can do with handlerInfos can be done with the RouteInfo type that is proposed in this RFC, with the benefit of sticking to supported public API. + +So we should deprecate willTransition and didTransition in favor of the following new events. + +### New Events: routeWillChange & routeDidChange + +The `routeWillChange` event fires whenever a new route is chosen as the desired target of a transition. This includes `transitionTo`, `replaceWith`, all redirection for any reason including error handling, and abort. Aborting implies changing the desired target back to where you already were. Once a transition has completed, `routeDidChange` fires. + +Both events receive a single `transition` argument as described in the "Transition Object" section below, which explains the meaning of `from` and `to` in more detail. + +Redirection example: + + 1. current route is A + 2. user initiates a transition to B + 3. routeWillChange fires `from` A `to` B. + 4. B redirects to C + 5. routeWillChange fires `from` A `to` C. + 6. routeDidChange fires `from` A `to` C. + +Abort example: + + 1. current route is A + 2. user initiates a transition to B + 3. routeWillChange fires `from` A `to` B. + 4. in response to the previous routeWillChange event, the transition is aborted. + 5. routeWillChange fires `from` A `to` A. + 8. routeDidChange fires `from` A `to` A. + +Error example: + + 1. current route is A + 2. user initiates a transition to B.index + 3. routeWillChange fires `from` A `to` B. + 4. B throws an exception, and the router discovers a "B-error" template. + 5. routeWillChange fires `from` A `to` B-error + 6. routeDidChange fires `from` A `to` B-error + +These are events, not extension hooks -- now that we are exposing a Service, it makes more sense to subscribe to its events than extend it. ### New Properties @@ -348,7 +385,7 @@ A more complete version that also matches models and queryParams can be written # Drawbacks -This RFC doesn't deprecate any public API, so the API-churn burden may appear low. However, we know that use of the private APIs we're deliberately disabling is widespread, so users will experience churn. We can provide our usual deprecation cycle to give them early warning, but it still imposes some cost. +This RFC deprecates only two public extension hooks API, so the API-churn burden may appear low. However, we know that use of the private APIs we're deliberately disabling is widespread, so users will experience churn. We can provide our usual deprecation cycle to give them early warning, but it still imposes some cost. This RFC doesn't attempt to change the existing and fairly rich semantics for initiating transitions. For example, you can pass either models or IDs, and those have subtle semantic differences. I think an ideal rewrite would also change the semantics of the route hooks and transitionTo to simplify that area. @@ -400,6 +437,3 @@ Should we introduce new API via the `Ember` global and switch to a module export They let you do spooky-action-at-a-distance stuff. We could instad choose to implement single-purpose methods like `(get-route-info)` and `{{#with-route-info someValue}}` that would solve the routing case without opening the door to arbitrary user-defined ones. -## Possible semantic breakage of willTransition - -I proposed fixing willTransition to ensure that it fires during redirects. I see that as a clear bugfix, but if there is reason to believe it will break people's apps then we should deprecate `willTransition` and introduce a new event with the correct semantics. From 24026235391e059f67dd1ccbe4a73b8f501f9879 Mon Sep 17 00:00:00 2001 From: Ricardo Mendes Date: Tue, 23 Aug 2016 15:02:36 +0100 Subject: [PATCH 12/16] typos --- text/0000-router-service.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/0000-router-service.md b/text/0000-router-service.md index 81805b8c37..1040e5e188 100644 --- a/text/0000-router-service.md +++ b/text/0000-router-service.md @@ -28,7 +28,7 @@ A `RouterService` is great for asking global questions, but some questions are n - `{{link-to}}` has a lot of complexity and performance cost that deals with changing its active state, and the precise timing of when that should happen. - - there is no way to ask the router what it would do to handle a given URL without actually visting that URL. + - there is no way to ask the router what it would do to handle a given URL without actually visiting that URL. All of the above can be addressed by embracing what is already internally true: "the current route" is not a single global, it's a dynamically-scoped variable that can have different values in different parts of the application simultaneously. @@ -232,7 +232,7 @@ router.currentRoute.find(info => info.name === 'people').params ## RouteInfoWithAttributes -This type is almost identical to `RouteInfo`, except it has one additional proprty named `attributes`. The attributes contain the data that was loaded for this route, which is typically just `{ model }`. +This type is almost identical to `RouteInfo`, except it has one additional property named `attributes`. The attributes contain the data that was loaded for this route, which is typically just `{ model }`. ## Transition Object From b958d6bdd0b6075e1bf63241e12b7177e452d563 Mon Sep 17 00:00:00 2001 From: Ricardo Mendes Date: Tue, 23 Aug 2016 15:27:41 +0100 Subject: [PATCH 13/16] Update 0000-router-service.md --- text/0000-router-service.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-router-service.md b/text/0000-router-service.md index 1040e5e188..37ea6b6998 100644 --- a/text/0000-router-service.md +++ b/text/0000-router-service.md @@ -435,5 +435,5 @@ Should we introduce new API via the `Ember` global and switch to a module export ## Generic dynamically-scoped variables may be too powerful -They let you do spooky-action-at-a-distance stuff. We could instad choose to implement single-purpose methods like `(get-route-info)` and `{{#with-route-info someValue}}` that would solve the routing case without opening the door to arbitrary user-defined ones. +They let you do spooky-action-at-a-distance stuff. We could instead choose to implement single-purpose methods like `(get-route-info)` and `{{#with-route-info someValue}}` that would solve the routing case without opening the door to arbitrary user-defined ones. From cd1eb692ca6c665a6de7f98ed773b0dec0888a77 Mon Sep 17 00:00:00 2001 From: Tom Dale Date: Tue, 23 Aug 2016 14:09:29 -0400 Subject: [PATCH 14/16] Add missing closing backticks --- text/0000-router-service.md | 1 + 1 file changed, 1 insertion(+) diff --git a/text/0000-router-service.md b/text/0000-router-service.md index 37ea6b6998..58da33478e 100644 --- a/text/0000-router-service.md +++ b/text/0000-router-service.md @@ -379,6 +379,7 @@ A more complete version that also matches models and queryParams can be written {{outlet}} {{/with-dynamic-variable}} {{/liquid-bind}} +``` 4. Prerendering of non-current routes becomes possible. You can use `recognizeAndLoad` to obtain a `RouteInfoWithAttributes` and then use `{{#with-dynamic-variable "routeInfo" myRouteInfo}} {{outlet}} {{/with-dynamic-variable}}` to render it. From 3ed17558c149bdde0691fa9028caede92ac4e79b Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Tue, 18 Oct 2016 17:05:09 -0400 Subject: [PATCH 15/16] incorporating suggestion to expose route parameter order --- text/0000-router-service.md | 1 + 1 file changed, 1 insertion(+) diff --git a/text/0000-router-service.md b/text/0000-router-service.md index 58da33478e..b3988c51a7 100644 --- a/text/0000-router-service.md +++ b/text/0000-router-service.md @@ -220,6 +220,7 @@ A RouteInfo object has the following properties. They are all read-only. - name: the dot-separated, fully-qualified name of this route, like `"people.index"`. - localName: the final part of the `name`, like `"index"`. - params: the values of this route's parameters. Same as the argument to `Route`'s `model` hook. Contains only the parameters valid for this route, if any (params for parent or child routes are not merged). + - paramNames: ordered list of the names of the params required for this route. It will contain the same strings as `Object.keys(params)`, but here the order is significant. This allows users to correctly pass params into routes programmatically. - queryParams: the values of any queryParams on this route. - parent: another RouteInfo instance, describing this route's parent route, if any. - child: another RouteInfo instance, describing this route's active child route, if any. From aea91e340df658600c0f20a3d9cce4fa5bc44981 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Tue, 18 Oct 2016 17:22:40 -0400 Subject: [PATCH 16/16] updates to remove general-purpose dynamically-scoped variables --- text/0000-router-service.md | 65 +++++++++++++++++-------------------- 1 file changed, 29 insertions(+), 36 deletions(-) diff --git a/text/0000-router-service.md b/text/0000-router-service.md index b3988c51a7..e66f20f299 100644 --- a/text/0000-router-service.md +++ b/text/0000-router-service.md @@ -10,7 +10,9 @@ This RFC proposes: - codifying and expanding the supported public API for the `transition` object that is currently passed to `Route` hooks. - - introducing the concept of dynamically-scoped routing info. + - introducing the `get-route-info` template helper + - introducing the `#with-route-info` template keyword + - introducing the `readsRouteInfo` static property on `Component` and `Helper`. These topics are closely related because they share a unified `RouteInfo` type, which will be described in detail. @@ -292,69 +294,64 @@ Some of the private APIs we should mark and warn include: - `lookup('router:main')` (should use `service:router` instead) -## Dynamically-Scoped Variables +## Dynamically-Scoped Route Info -The next section introduces general-purpose, dynamically-scoped variables. Why am I doing this in "The Router Service RFC"? Because dynamically-scoped RouteInfo has several nice capabilities that will be introduced in the subsequent section. And once we have one dynamically-scoped variable, we have the option of exposing that capability for other uses. +"The current route" is not a global value -- it varies from place to place within an application. Internally, Ember already models route info as a dynamically-scoped variable (currently named `outletState`). This RFC proposes publicly exposing that value in order to make things like `link-to` easier to implement directly on public primitives, and in order to enable stable public API for addons usage like `{{liquid-outlet}}`. -### In the general case - -A [dynamically-scoped variable](https://en.wikipedia.org/wiki/Scope_(computer_science)#Dynamic_scoping) takes its value from its calling context, as opposed to its lexical context. Since dynamically-scoped variables are powerful to the point of potential danger, their syntax is intended to be appropriately verbose. - -Reading a dynamically-scoped variable in handlebars: +We propose `get-route-info` for reading the current route info in handlebars: ```hbs {{!- retrieve the value of a dynamically scoped variable }} - (get-dynamic-variable "myVariableName") + {{some-component currentRoute=(get-route-info)}} ``` -Defining a component that reads a dynamically-scoped variable: +We propose `readsRouteInfo` for defining a component that reads route info: ```js let MyComponent = Ember.Component.extend({ didInsertElement() { - // Access to the variable here is intended to be indistinguishable + // Accessing routInfo here is intended to be indistinguishable // from a normal, explicitly-passed input argument. - doSomethingWith(this.get('myDynamicVariable')); + doSomethingWith(this.get('routeInfo')); } }); MyComponent.reopenClass({ - // This is where the specialness happens. - getDynamicVariables: [ 'myDynamicVariable' ] + // This is where we declare that we need access to routeInfo + readsRouteInfo: true }); ``` -Defining a helper that reads a dynamically-scoped variable: +And `readsRouteInfo` also works on `Helper`: ```js let MyHelper = Ember.Helper.extend({ compute(params, hash) { - return doSomethingWith(hash.myDynamicVariable); + // routeInfo is indistinguishable from a normally-passed hash argument + return doSomethingWith(hash.routeInfo); } }); MyHelper.reopenClass({ - getDynamicVariables: [ 'myDynamicVariable' ] + readsRouteInfo: true }); ``` -Setting a dynamically-scoped variable: +We propose the `#with-route-info` keyword for setting a new route info: ```hbs -{{#with-dynamic-variable "myVariableName" someValue}} +{{#with-route-info someValue}} {{!- within this block AND ALL ITS DESCENDANTS until - otherwise overridden by another with-dynamic-variable statement, - `get-dynamic-variable "myVariableName"` returns someValue. + otherwise overridden by another set-route-info statement, + `get-route-info` returns someValue. -}} -{{/with-dynamic-variable}} +{{/with-route-info}} ``` -Note that there is no `set-dynamic-variable`. You can only introduce new scopes, not mutate your containing scope. There is also no way to set a dynamically-scoped variable directly from Javascript -- your component must use a `with-dynamic-variable` block within its handlebars template. - -User-defined, dynamically-scoped variables must be pre-declared before first render via `Ember.declareDynamicVariable("myVariableName")`. Attempting to set one that wasn't declared is an error. Attempting to read one that doesn't exist returns `undefined`. Predeclaration is an optimization that allows us to maintain consistent performance within Glimmer. +Note that there is no `set-route-info`. You can only introduce new scopes, not mutate your containing scope. There is also no way to set routeInfo directly from Javascript -- your component must use a `with-route-info` block within its handlebars template. -### The specific case of routeInfo +### routeInfo's type, and examples -`routeInfo` is a dynamically-scoped variable provided by Ember as public API. Its value is always a `RouteInfoWithAttributes` object that is correct for the given route context. This enables several nice things, which I will illustrate with examples: +The value returned from `get-route-info` and acceptd by `with-route-info` is always a `RouteInfoWithAttributes` object. This enables several nice things, which I will illustrate with examples: 1. Here is a simplified `is-active` helper that will always update at the appropriate time to match exactly what is rendered in the current outlet. It will maintain the correct state even during animations. Instead of injecting the router service, it consumes the `routeInfo` from its containing environment: @@ -364,7 +361,7 @@ Ember.Helper.extend({ return !!routeInfo.find(info => info.name === routeName); } }).reopenClass({ - getDynamicVariables: ['routeInfo'] + readsRouteInfo: true }); ``` @@ -375,14 +372,14 @@ A more complete version that also matches models and queryParams can be written 3. `liquid-outlet` can be implemented entirely via public API. It would become: ```hbs -{{#liquid-bind (get-dynamic-variable "routeInfo") as |currentRouteInfo|}} - {{#with-dynamic-variable "routeInfo" currentRouteInfo}} +{{#liquid-bind (get-route-info) as |currentRouteInfo|}} + {{#with-route-info currentRouteInfo}} {{outlet}} - {{/with-dynamic-variable}} + {{/with-route-info}} {{/liquid-bind}} ``` -4. Prerendering of non-current routes becomes possible. You can use `recognizeAndLoad` to obtain a `RouteInfoWithAttributes` and then use `{{#with-dynamic-variable "routeInfo" myRouteInfo}} {{outlet}} {{/with-dynamic-variable}}` to render it. +4. Prerendering of non-current routes becomes possible. You can use `recognizeAndLoad` to obtain a `RouteInfoWithAttributes` and then use `{{#with-route-info myRouteInfo}} {{outlet}} {{/with-route-info}}` to render it. # Drawbacks @@ -435,7 +432,3 @@ Should we introduce new API via the `Ember` global and switch to a module export import { DEFAULT_VALUE } from 'ember-routing'; -## Generic dynamically-scoped variables may be too powerful - -They let you do spooky-action-at-a-distance stuff. We could instead choose to implement single-purpose methods like `(get-route-info)` and `{{#with-route-info someValue}}` that would solve the routing case without opening the door to arbitrary user-defined ones. -