Create Router Service #95

ef4 · 2015-09-24T22:35:33Z

chadhietala · 2015-09-25T00:25:53Z

text/0000-router-service.md

+
+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"`.


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.

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 model hooks. There would be no need to do a new top-down query building step -- the hooks we have are already where routes tell us what data they need.

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 modelFor return a promise, so that you can re-introduce explicit dependencies when you need them, and (3) provide an optional way to declare a route as blocking, which can still be necessary if you're going to do data-dependent redirections.

Agree on all points.

👍 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.

I think they are fine being internally/privately stateful, but not good being singletons.

machty · 2015-09-27T23:07:48Z

A few random thoughts/notes:

Perhaps we should role in some of the details of the routeable components RFC into here, specifically with shrinking the Route hook API to just a model that's always called (with objects passed to transitionTo getting serialized into params passed to the always-called model).
I want to noodle a little more on what a transition is; transitions manage a mess of async and I would want to minimize via one mechanism or another the ability to reach into it and twiddle with its internals, particularly the progressively-resolving internal handlerInfos
We need a way to grab the current router state such that it can be transitioned to (returned to) later

MiguelMadero · 2015-09-28T06:00:24Z

@ef4 we have a similar service now. There're two things that we're using that could be part of the public interface. It's easy to build those on top of the current public interface, but something to consider.

    /**
     * Transitions to an ancestor. For example from charts.
     */
    transitionUp: function () {
        var segments = this.get('currentRouteName').split('.');
        segments.pop();
        this.transitionToRoute(segments.join('.'));
    },

    _subscribeToRouterEvents: function () {
        var router = this.get('router');
        router.on('didTransition', this._handleDidTransition.bind(this));
    }.on('init'),

    _handleDidTransition: function () {
        this.trigger('routerDidTransition');
    }

Our RoutingService is also Ember.Evented, so a consumer can subscribe to the routerDidTransition event to know when there're route changes. We use this from an analytics service and to communicate with our native (ios/android) wrapper.

nathanhammond · 2015-10-06T21:37:03Z

text/0000-router-service.md

+
+### 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.


Dropping didTransition and willTransition is likely going to be very painful. Basically anybody who ever implemented analytics used those hooks. Some people even (read, me) stored stuff on the transition object and pulled it back out later.

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.

—
Sent from Mailbox

On Tue, Oct 6, 2015 at 2:37 PM, Nathan Hammond notifications@github.com
wrote:

+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.

Dropping didTransition and willTransition is likely going to be very painful. Basically anybody who ever implemented analytics used those hooks. Some people even (read, me) stored stuff on the transition object and pulled it back out later.

Reply to this email directly or view it on GitHub:
https://github.com/emberjs/rfcs/pull/95/files#r41325988

willTransition is especially helpful for globally preventing a route based on some global state. Say for example pending file uploads. Something the "routable component" might not be aware of.

@ef4 I think we likely want both evented and observable here. It seems like different use-cases would prefer to "subscribe" in different ways.

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 beforeTransition or whatever the event would be and could interject their thoughts.
The first thing that comes to mind is a user-form component within a users.create route, while the route does not know the user-form's state until submission (DDAU) the form could listen for the transition and check to see if any data could be lost and alert/confirm, etc.

@ef4 By deprecating the willTransition and didTransition hooks, do you mean that the willTransition and didTransition events would still be available (or something akin to them at least)? Would this be the time/place to request a changeTransition event (name up for debate) that would be triggered for transitions initiated between willTransition and didTransition?

blimmer · 2015-10-07T01:29:22Z

Overall, I really like this idea! I think a routing service is very helpful an will make my previous apps use less private APIs. I think the only downside is consumers relying on this service to not handle routing concerns in the Ember.Route they're currently on, which the current API encourages.

chadhietala · 2015-10-11T18:42:49Z

text/0000-router-service.md

+
+```js
+transitionTo(routeName, ...models, queryParams)
+replaceWith(routeName, ...models, queryParms)


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 window references because the framework didn't provide a good way of handling those cases with a good abstraction.

I suspect this would be a tangental RFC

kellyselden · 2015-10-12T17:49:39Z

I got linked here in slack, that this RFC might be curious how people are using currentHandlerInfos. Here is my relevant piece of code from ember-cli-transition-to-dynamic:

  this.router.router.currentHandlerInfos.forEach(route => {
    var routeSegments = routesWithSegments[route.name];

    // using route._names instead of route.params because
    // the latter's order is not guaranteed
    route._names.forEach(param => {
      var paramsSource = routeSegments && routeSegments[param]
        ? routeSegments : route.params;
      models.push(paramsSource[param]);
    });
  });

stefanpenner · 2015-10-12T18:16:00Z

text/0000-router-service.md

+ - 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:


I'm uncertain about Enumerable future, I believe we should transition to iterable, as that isn't something we have done yet, i suspect we should leave out the enumerable part here. We can always expose iterability etc. at a later date.

I believe with this linked list you can get what router.router.currentHandlerInfos used to get you. I also use route._names as an ordered version of route.params mentioned here. Is this something that is covered in the RFC? Is the new RouteInfo.params ordered?

nickiaconis · 2016-01-05T19:50:11Z

@ef4 -- By deprecating the willTransition and didTransition hooks, do you mean that the willTransition and didTransition events would still be available (or something akin to them at least)?

Also, would this be the time/place to request a changeTransition event (name up for debate) that would be triggered for transitions initiated between willTransition and didTransition (i.e. when one route redirects to another before didTransition is fired)?

kellyselden · 2016-10-18T17:34:22Z

I added some comments a couple weeks ago. I'm unfamiliar with final comment period. Are those comments now blocking the merge, or will the merge proceed with unaddressed comments?

ef4 · 2016-10-18T18:32:16Z

@kellyselden I missed your open question about param ordering.

params is a POJO of keys and values (it's the same thing that gets passed into a Route's model hook), so it's not ordered. We can add a paramNames to RouteInfo that would b an array of string parameter names, so that it's possible to determine what parameters are required for the route and in what order.

This seems like a simple and uncontroversial bit of detail to add, thanks for pointing it out.

ef4 · 2016-10-18T18:35:46Z

After taking feedback and hearing a lot of uncertainty around general-purpose dynamic variables, I'm going to table them in favor of get-route-info and set-route-info which the RFC mentions in Alternatives under "Generic dynamically-scoped variables may be too powerful". They serve the exact same purpose for routing, without opening the door to non-routing uses. We can do general-purpose dynamic variables in a separate RFC.

nathanhammond · 2016-10-18T22:25:34Z

Reviewing the original implementation of the query-params subexpression we have always disallowed passing of POJOs.

I'd like to add a rider to this RFC which allows the subexpression to receive objects. It becomes incredibly annoying in the scenario where we don't investigate the final state to prune values from the query string inside of Handlebars files and makes it more difficult to accomplish standardization of nested query param behavior since everything must still be key/value pairs.

Current State:

{{#link-to 'some-route' (query-params
  key1="value1"
  key2="value2"
  key3=(if key3 key3)
)}}Some Route{{/link-to}}
{{! key3 if statement required in order to prevent it from being in the output if `undefined`. }}
{{! Serialization happens prior to removal. }}

Proposed Approach:

Ember.controller.extend({
  someQPObject: {
    key1: 'value1',
    key2: 'value2'
  }
});

This would make it sane to adopt my proposed nested query params specification as a unifying pattern across all of our layers:

{{#link-to 'some-route' (query-params someQPObject)}}Some Route{{/link-to}}

Sketch to update current implementation:

positional.forEach(function(positionalArg) { if (typeof positionalArg !== "object") { assert(); } });
var result = Ember.assign({}, ...positional, named.value());

I'm also amenable to disallowing mixed usage and only allowing a single positional param:

if (positional.length === 1 && typeof positional === "object" && Object.keys(named).length === 0) { return positional[0]; }

if (positional.length === 0) { return Ember.assign({}, named.value()); }

ef4 · 2016-10-19T02:27:28Z

@nathanhammond that all sounds good but this RFC is already pretty big and thoroughly argued, and I was hoping not to have to restart final comment period (since my latest updates were all just incorporating feedback from the discussion so far and AFAIK there are no controversial questions outstanding).

Done separately, the query params change seems like a short and sweet RFC that can sail through.

nathanhammond · 2016-10-19T06:58:27Z

text/0000-router-service.md

+
+ - 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=`.


Rereading this now with a bit more context and explaining it for future travelers (read: me, six months from now).

This is to ensure that our default behavior of "hydrating unsupplied query params" continues to work. Without this we effectively end up doing Object.assign({}, unsupplied, queryParams); which would make queryParams only additive, which is wrong. We can't use values other than this symbol as it is possible that it may be valid user input.

I'm concerned about the name as DEFAULT_VALUE implies more than we likely mean to convey. I'd nominate UNSET_VALUE, UNBIND_VALUE, or DELETE_VALUE instead.

Can we stash this somewhere not on the Ember global?

Also,

This approach for unsetting does not completely support nested (structured) array query params. It's not easy to model unset behavior in this scenario: { foo: [ "a", "b", "c"] } => foo[]=a&foo[]=b&foo[]=c. That should instead do some sort of slice, so maybe some sort of:

{ foo: SLICE(SLICE(ORIGINAL,0,1),0,1) } I believe that this needs to support nesting to be complete.

(This maps to my forthcoming serialization unification proposal.)

This gets worse if you nest arrays of objects and want to UNSET a property on a nested value.

{ characters: [ { thing: "one", hat: false }, { thing: "two", hat: false }, { thing: "cat", hat: true } ] }

Indices get included because otherwise the array is ambiguous.

characters[0][thing]=one&characters[0][hat]=0& characters[1][thing]=two&characters[1][hat]=0& characters[2][thing]=cat&characters[2][hat]=1

If I wish to unset characters[2][hat] and eliminate characters[0][thing]=one (never liked that one anyway) I'm unsure it's possible to do gracefully with this set of proposed tools.

I think structured query params are additive API, so unless there is something in this proposal that forecloses on your design, I think we don't need to completely solve it here.

They're additive in that they're new but considering our current state has severe issues I want to ensure that we feel good about our future direction.

For the basic scenario and only-keyed-objects I think we can get away with DELETE_VALUE. To support arrays it almost has to become a Lisp where you specify the transforms. To do that gracefully I feel like we would need to adopt:

push, pop

shift, unshift

slice

splice

...

And that's beyond what I would want to support as it would come with a parser and all sorts of other annoying problems. Throwing away that proposal, I think it's bad.

We could always stamp something as "pre-merged" and expose an API to get the unsupplied values. It'll make structured query param merging palatable for advanced users, doesn't impact this simple scenario, doesn't add a tremendous amount of complexity and another Lisp-y microsyntax.

For bonus points I don't think that needs to be specified at this time and seems like a direction where this doesn't paint us into any corners.

/FIN

locks · 2016-10-27T02:19:57Z

I would like to thank everyone involved for shaping this RFC into its current state through detailed and constructive feedback.

This service has been a long time coming, and we hope that transitioning from the current intimate API is both swift and easy. On top of that, it will unlock new capabilities and optimization opportunities.

In this Final Comment Period only one new concern was raised, and it was considered that it is an additive concern that can be addressed by a follow up RFC to the present one. So all that's left to do is to merge it. 🎉

One final thank you to @ef4 for sticking with it for the last year and change!

MiguelMadero · 2017-02-01T23:36:51Z

Would it make sense to consider hasRoute as part of the public API?
Today it's available on the ember router, router.js and the recognizer.

cc: @kazuzenefits

rwjblue · 2017-02-02T14:49:29Z

@MiguelMadero - I think it seems fine, but it should be done as a follow-up RFC. It should be small enough and be able to focus on just this one method...

MiguelMadero · 2017-02-03T00:55:20Z

Thanks @rwjblue I can throw something together. I just wanted to check first.

Fed03 · 2017-07-05T16:39:56Z

I think would be handy if we can expose methods like transitionBack and transitionForward handled by the service in order to navigate inside the ember app boundaries. what do you think?

sdhull · 2017-12-07T20:46:38Z

Not sure if this is the right place to add comments for continuing work on the router service or not (considering this was merged but doesn't represent the entirety of what was described in https://github.com/emberjs/rfcs/blob/master/text/0095-router-service.md).

Anyway my 2c:
Rendering links for pagination is currently difficult in ember because there is no equivalent of what you find in Rails url_for(params.merge(page: new_page)). We get close with routerService.urlFor and RouteInfo, however there is no easy way (afaict) to get all the params you need to feed into urlFor.

What I would like to see is the addition of something like RouteInfo#urlArgs (or some other name?) that would return an array containing RouteInfo#name (dot-separated fully-qualified name, eg 'people.show.posts.show.comments') & any models or model ids used to generate this route. Then you could do:

const queryParams = routeInfo.queryParams;
queryParams.page = newPage;
routerService.urlFor(...routeInfo.urlArgs, {queryParams: queryParams});

I guess alternatively RouteInfo#models would also be sufficient, as long as it includes all the models (or ids) required for the give RouteInfo#name.

Standarize targets

…4199) * [ember__routing] Add types from Router Service RFC emberjs/rfcs#95 * [ember__routing] Add `RouteInfo` tests Also includes tweaks to `RouteInfo` definition and docs

Create Router Service

df29bdc

chadhietala reviewed Sep 25, 2015
View reviewed changes

ef4 mentioned this pull request Oct 5, 2015

[Route Prefetching] Allow routes to request data in parallel #97

Closed

nathanhammond reviewed Oct 6, 2015
View reviewed changes

chadhietala reviewed Oct 11, 2015
View reviewed changes

stefanpenner reviewed Oct 12, 2015
View reviewed changes

jcope2013 mentioned this pull request Oct 14, 2015

Routable Components RFC #38

Closed

rwjblue mentioned this pull request Oct 18, 2015

Attribute Actions #100

Closed

mixonic mentioned this pull request Nov 10, 2015

action does not bubble from controller emberjs/ember.js#12581

Closed

rwjblue mentioned this pull request Dec 6, 2015

Allow developers to declare a "dummy" link-to #106

Closed

machty mentioned this pull request Jan 8, 2016

[DOC Release] Mark multiple methods in Ember.Router as public emberjs/ember.js#12763

Closed

ef4 added 2 commits October 18, 2016 17:05

incorporating suggestion to expose route parameter order

3ed1755

updates to remove general-purpose dynamically-scoped variables

aea91e3

nathanhammond reviewed Oct 19, 2016

View reviewed changes

locks merged commit e1c1acb into master Oct 27, 2016

rwjblue mentioned this pull request Oct 27, 2016

Dynamic var params emberjs/ember.js#14357

Closed

ef4 mentioned this pull request Dec 27, 2016

Implicit parameters to link-to break during animation ember-animation/liquid-fire#347

Open

locks deleted the router-service branch January 30, 2017 15:07

GavinJoyce mentioned this pull request Mar 18, 2017

Is it still needed with Glimmer 2 and Ember 2.12? intercom/ember-href-to#72

Closed

Leooo mentioned this pull request Jun 25, 2017

compatibility with route-action ember-animation/liquid-fire#578

Open

nickiaconis mentioned this pull request Aug 11, 2017

[BUGFIX Release] fix memory leak nickiaconis/ember-prefetch#86

Merged

chadhietala mentioned this pull request Aug 30, 2018

Router(js) refactor (wip) emberjs/ember.js#16838

Closed

chadhietala mentioned this pull request Sep 18, 2018

[Breaking] RouteInfo Type And Transition.(from|to) tildeio/router.js#252

Merged

This was referenced Oct 1, 2018

[META] Routing Service "Go" Items emberjs/ember.js#17027

Closed

Add recognize and recognizeAndLoad tildeio/router.js#261

Merged

ef4 mentioned this pull request Jan 15, 2019

Fix router event deprecations, update to Ember 3.7 DockYard/ember-router-scroll#183

Merged

kategengler pushed a commit that referenced this pull request Jan 19, 2019

Merge pull request #95 from cibernox/standarize-targets

9b94a35

Standarize targets

tleperou mentioned this pull request Jul 6, 2019

Add load hook to Route #510

Closed


		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"`.


		### 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.


		- 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=`.

Create Router Service #95

Create Router Service #95

Conversation

ef4 commented Sep 24, 2015 • edited by chadhietala Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machty commented Sep 27, 2015

MiguelMadero commented Sep 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dropping didTransition and willTransition is likely going to be very painful. Basically anybody who ever implemented analytics used those hooks. Some people even (read, me) stored stuff on the transition object and pulled it back out later.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blimmer commented Oct 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kellyselden commented Oct 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickiaconis commented Jan 5, 2016

kellyselden commented Oct 18, 2016

ef4 commented Oct 18, 2016

ef4 commented Oct 18, 2016

nathanhammond commented Oct 18, 2016 • edited Loading

ef4 commented Oct 19, 2016

nathanhammond Oct 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathanhammond Oct 19, 2016 • edited Loading

Choose a reason for hiding this comment

locks commented Oct 27, 2016

MiguelMadero commented Feb 1, 2017

rwjblue commented Feb 2, 2017

MiguelMadero commented Feb 3, 2017

Fed03 commented Jul 5, 2017

sdhull commented Dec 7, 2017

ef4 commented Sep 24, 2015 •

edited by chadhietala

Loading

Dropping `didTransition` and `willTransition` is likely going to be very painful. Basically anybody who ever implemented analytics used those hooks. Some people even (read, me) stored stuff on the transition object and pulled it back out later.

nathanhammond commented Oct 18, 2016 •

edited

Loading

nathanhammond Oct 19, 2016 •

edited

Loading

nathanhammond Oct 19, 2016 •

edited

Loading