-
-
Notifications
You must be signed in to change notification settings - Fork 408
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
Create Router Service #95
Changes from 1 commit
df29bdc
edc3723
e194836
fc06b8b
489fbee
dc7e38d
ca250a0
7e29f4a
eee39cc
84f5239
6b430dc
e3bfde6
01ad76a
2402623
8d3bf65
b958d6b
cd1eb69
3ed1755
aea91e3
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,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) | ||
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 it might be useful to also introduce more APIs here to get closer to parody with the underlying browser API. For instance it's currently not very clear how to go back a transition. Solving this in user space normally requires you to save off the previous transition to create an adhoc linked list across your app or create a service to keep track of this, which may be subject to getting out of sync. Things get more leaky when you are in a node environment and now need to guard against 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 suspect this would be a tangental RFC |
||
``` | ||
|
||
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() { | ||
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. Even in ES6 this needs a property name: 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. Additionally, this should be 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. does |
||
this.recompute(); | ||
}); | ||
}); | ||
``` | ||
|
||
```hbs | ||
{{!- Example usage -}} | ||
<li class={{if (is-active "person.detail" model) 'chosen'}} > | ||
|
||
{{!- Example usage with generic routeName and list of models (avoids splat) -}} | ||
<a class={{if (is-active routeName models=models) 'chosen'}} > | ||
|
||
{{!- Note that the complexities of currentWhen can be avoided by composing instead. }} | ||
<a class={{if (or (is-active 'one') (is-active 'two')) 'active'}} href={{url-for 'two'}} > | ||
|
||
``` | ||
|
||
|
||
### 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. | ||
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. Implementing this as an observer will mean that you have to wire up your own memoization which was previously easy to do by having the (Not saying that these API changes would prevent any of the existing use patterns, just some of them would be a bit harder to implement.) 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.
observable property, doesn't mean observer. Rather it is just a property that is guaranteed which correctly emits change events. Allowing CP (or maybe observer) to depend. The 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. @ef4 currentRoute should likely be readOnly. we may also want to explore using Object.freeze or similar . |
||
|
||
`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. | ||
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. Dropping 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 agree. Maybe we need to provide a new event that doesn't expose private members. It also seems better than relying on an observer. — On Tue, Oct 6, 2015 at 2:37 PM, Nathan Hammond notifications@github.com
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.
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. @ef4 I think we likely want both evented and observable here. It seems like different use-cases would prefer to "subscribe" in different ways. 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. To check would the proposed observable/event be on the router service or the private router? Seems like an interesting thought to have components subscribe to 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. @ef4 By deprecating the |
||
|
||
## 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"`. | ||
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. Having the name is great but do you think it would be worth giving the instance or the class as well? I would imagine you could use ember with something that allows you to specify "query fragments" (Falcor) in your routes and potentially components, if you have a way of collecting those fragments recursively. For example you could imagine that in node you could do something like this and prefetch all the data for the initial route: let indexHTML = fs.readFileSync(__dirname+'/index.html').toString();
const dataRegex = /%DATA%/;
const buildQuery = (handler, query = []) => {
query = query.concat(handler.instance.collectQuery());
if (handler.parent) {
buildQuery(handler.parent, query)
}
return callUpstreamService(query)
}
app.collectQueryFor('/profile/123').then(buildQuery).then(data => {
let indexHTML = indexHTML.replace(dataRegex, JSON.stringify(data));
res.writeHead(200, {
'Content-Length': indexHTML.length,
'Content-Type': 'text/html'
});
res.write(indexHTML);
res.end();
}) This would allow you to extract the data requirements without a lot of overhead. If not, this can be proved out in user land as you should be able to lookup the route instances from the info. 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 don't want to encourage people to do anything remotely stateful with Route instances. I think it's dubious that they are instances at all. Your overall scenario is an important one, and it comes down to enabling routes to resolve their promises in parallel instead of in series. That would let your data layer be Falcor or GraphQL-like and automagically batch and combine the requests generated in each of the That whole feature is probably a different RFC. I think it would (1) cause routes to be non-blocking by default (meaning their children's model hooks begin to run even before their own model hook resolves), (2) make 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. Agree on all points. 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. 👍 on routes not being instances. Making them stateful prevents all sorts of cleverness for solving exactly the problem Chad brings up. The separate RFC is #97. 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 they are fine being internally/privately stateful, but not good being singletons. |
||
- 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 }`. | ||
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. No |
||
- 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: | ||
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 uncertain about Enumerable future, I believe we should transition to 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 believe with this linked list you can get what |
||
|
||
```js | ||
router.currentRoute.find(info => info.name === 'people').params | ||
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.
A lazy 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. Ah, I see later on where you want to traverse |
||
``` | ||
|
||
## 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. | ||
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. when in a series of transitions, with aborts/redirects/retry's will |
||
|
||
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. | ||
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. This is so needed. Right now I'm relying on two (private) layers deep in router.js to do this. 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. having to write guards for |
||
|
||
### 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)) { | ||
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.
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. Alternatively, it is guaranteed to be unique, but we end up leaking the mount point of the engine into the engine allowing for all sorts of bad behavior. |
||
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. | ||
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 those deprecations will likely affect analytics/instrumentation related add-ons (or apps that use them directly). and replicating that class of behavior via observable property feels quirky. I think the two hook deprecations need some further exploration. |
||
|
||
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. | ||
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. seems good to keep those separate. |
||
|
||
# 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". | ||
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. We likely need to take a survey of some real apps, and see how many of those on-transition properties are wildly used. I know I have touched them, when nothing else worked. The survey results likely wouldn't stop us from slowly phasing them out, rather to be sure we phase out "weird API" IFF a reasonable alternative is provided (unless we deem the scenario to esoteric ) |
||
|
||
## 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. | ||
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 am 👍 on having them as part of the framework (perhaps renamed to 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. agreed on both leaving them out, and requiring naming bikeshedding. |
||
|
||
## Branching Route Hierarchies | ||
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. This is something that I want to revisit; I had a conversation with @machty this summer about his stack routing research. It seems likely we want to try and solve this coincidentally to "rip the bandaid off" so to say. 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 this RFC creates enough public API that you could implement these things in userspace without crazy hacks. For example, to do a split-page app with two independent route hierarchies, your code could call |
||
|
||
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. | ||
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. What you describe here is basically the solution for modals we've all been waiting for. |
||
|
||
## 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; | ||
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. an alternative (or addition) to the general purpose |
||
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: | ||
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 would be useful to have every route in the hierarchy available via a public API. We use handlerInfos to get this information for tracking purposes right now. 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. Depending upon when you access it nothing below the current pivot is guaranteed to be "resolved." 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 believe our particular use case only needs the routes in the hierarchy above the pivot. In fact we do work to figure out where the pivot is, so it would actually be nicer for us to only get the routes above the pivot. |
||
|
||
```js | ||
beforeModel(transition) { | ||
const parentModel = this.modelFor(this.parentRouteName()); | ||
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. seems like it could be a property? |
||
} | ||
``` | ||
Possibly we *want* this to feel awkward because it's a weird thing to do. | ||
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 asked how to do this all the time but usually there is a better solution for whatever the presented problem is than trying to determine the parent route name dynamically. I'm okay with it being awkward, it's still a lot cleaner than diving into |
||
|
||
|
||
|
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.
EmberRouter
's publicwillTransition
anddidTransition
API include the parametersoldInfos
andnewInfos
, in addition to (and preceding) thetransition
parameter.https://github.com/emberjs/ember.js/blob/v2.7.0/packages/ember-routing/lib/system/router.js#L292
Will these be carried over as well?
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.
No, one of the explicit goals of this RFC is to codify public API so that people can stop messing around with handlerInfos (which are an internal implementation detail of router.js and are not Ember public API).