RouteInfo Metadata #398

chadhietala · 2018-11-02T13:29:46Z

Rendered

Co-Authored-By: rwjblue <[email protected]>

text/0000-RouteInfo-Metadata.md

Co-Authored-By: chadhietala <[email protected]>

Leooo · 2018-11-09T22:18:07Z

Something we found on our side is that it's very convenient to add and manage static meta data (titles, breadcrumbs titles, tracking info etc.) In the router's route declaration itself, rather than having to look at each indivual routes properties: https://github.com/britishgas-engineering/ember-router-meta

chadhietala · 2018-11-14T04:52:52Z

After our meeting on Friday we decided to move this into final comment period.

@Leooo interesting. I think the reason why I opted into putting it directly on the Route was that you may need to derive the return value based on values only available at runtime.

lolmaus · 2018-11-14T13:47:17Z

Hi! Got a number of questions regarding this.

Since the hook is fired before the beforeModel, how do I put model data into metadata?
How do I update metadata from a controller or a service?
How do I use computed properties in metadata? It would be nice to have a usage pattern that makes metadata update automatically when a CP recomputes.
How do I put metadata into a template? The init hook approach requires wa-a-ay too much boilerplate for a simple {{metadata.pageTitle}}.
How do I put metadata into <head>? It may be out of scope and there are addons covering this (which can start using this RFC), but still it would be nice for the RFC to provide some guidelines. Or maybe it could even supersede them entirely.

Leooo · 2018-11-14T13:53:23Z

@chadhietala something that the ember-router-meta addon allows is to define most static properties in the router, and override only when needed with some logic in the route, using the related service. We use it to set page names for example - where only a few of them are dynamic. It's pretty much perfect for our use case - it would have been so much a hassle to define page titles etc. in each route, while now we have the best of both worlds and most of the things are defined in the router like for example:

this.route('dashboard', {pageName: 'My Account', pageType: 'Account management',
          section: 'My Account', path: '/accounts', breadCrumb: 'My account', intent: 'Manage your account'}, function () {

Nice job @Mikek2252 btw

chadhietala · 2018-11-14T16:18:13Z

@lolmaus I have intentionally designed the API to dissuade stateful things to occur in the hook and it is meant to be read-only once it is created. This is to align with the other properties on the RouteInfo object as specified in in RFC#95. The data can be passed into component's through the readsRouteInfo as specified by RFC#95. The data would also be accessed in the template through {{get-route-info}} as specified by RFC#95. As I showed in the Appendix of the RFC you can just do document.title = routeInfo.metadata.title

lolmaus · 2018-11-14T20:12:25Z

@chadhietala Hmm, so since the data can only be static, the use of this RFC is quite limited?

Can it explain what it is intended for and what it isn't?

ro0gr · 2018-11-14T21:03:27Z

text/0000-RouteInfo-Metadata.md

+
+While the `RouteInfo` object is sufficient in providing developers metadata about the `Route` itself, it is not sufficient in layering on application specific metadata about the `Route`. This metadata could be anything from a more domain-specific name for a `Route`, e.g. `profile_page` vs `profile.index`, all the way to providing contextual data when the `Route` was visited.
+
+This metadata could be used for more pratical things like updating the `document.title`.


I think document.title part in the Motivation section is misleading a bit.

While a proposed stateless metadata design allows you to cover titles for static routes. It doesn't seem possible to support document.title for routes with dynamic params.

For example, let's say I have a route with a dynamic segment /products/:id. In this case I would expect to have a product.title(loaded in the model hook) included to the document.title.

I think this is actually possible with the combination of 2 Router Service. On the Router Service there is currentRoute which is a RouteInfoWithAttributes. This has the materialized model on it. So you can do:

export default Route.extend({ buildRouteInfoMetadata() { return { title(model) { return `Product: ${model.name}`; } } } // ... });

this.router.on('routeDidUpdate', (transition) => { document.title = transition.to.title(this.router.currentRoute.attributes); }) ``

Any time we lean on the global router.currentRoute in an event handler like this, we are implicitly making timing guarantees about the relative order of events.

Maybe that is OK and unavoidable, but it's worth thinking about alternatives that make the data dependency more explicit:

this.router.on('routeDidUpdate', async (transition) => { let model = await transition.to.withAttributes(); document.title = transition.to.title(model); })

(in which I'm imagining a new withAttributes(): Promise<RouteInfoWithAttributes> method on RouteInfo.)

There's a tradeoff here, because the simple thing introduces asynchrony, which then also invites a whole host of other issues.

Wait, is there any reason routeDidUpdate can't receive a transition that already has RouteInfoWithAttributes? It seems like by the time we're doing didUpdate we have the info.

Yes it could receive the RouteInfoWithAttributes

chadhietala · 2018-11-19T03:11:12Z

@ro0gr and @lolmaus To deal with shortcomings of not having access to the model data in a natural way we decided to change the type of the object that is passed on the Transition.to and Transition.from fields. They will now be RouteInfoWithAttributes which have the resolved model on them. This will allow you to do something like.

export default Route.extend({
  buildRouteInfoMetadata() {
    return {
      title(model) {
        return `Product: ${model.name}`;
      }
    }
  }
  // ...
});

this.router.on('routeDidUpdate', transition => {
  let model = transition.to.attributes;
  document.title = transition.to.metadata.title(model);
})

ro0gr · 2018-11-19T21:27:01Z

@chadhietala thanks for update. I like RouteInfoWithAttributes.

I have few more questions.

You've provided such an example:

document.title = transition.to.title(model);

I assume you meant

document.title = transition.to.metadata.title(model);

or does metadata affect a RouteInfo interface?

When I saw your example with a method defined on the metadata I've got curious which data types are permitted as a values in the metadata? I know metadata is readonly and frozen, however I think it still leaves an area for a possible state leakage:

 buildRouteInfoMetadata() {
    return {
      ['AmIDangerous?']: this.store
    }
  }

Should metadata be guarded against the primitive values only? Or can we just get rid of this when invoking a metadata hook?

chadhietala · 2018-11-20T19:51:12Z

@ro0gr Yes you are correct RE: transition.to.metadata.title(model);. Yea you can totally do something like that, but I think this may be a thing people may want to lint for.

jacobq · 2018-12-05T00:14:28Z

text/0000-RouteInfo-Metadata.md

+```js
+// app/route/profile.js
+import Route from '@ember/routing/route';
+import { inject as service } from '@ember/service';


typo: importing as the name service but using inject below

chadhietala and others added 2 commits November 2, 2018 09:28

RouteInfo Metadata

b82b4a8

Co-Authored-By: rwjblue <[email protected]>

Add document title example

526cfbb

locks requested changes Nov 2, 2018

View reviewed changes

locks and others added 20 commits November 5, 2018 17:10

Update text/0000-RouteInfo-Metadata.md

f469a74

Co-Authored-By: chadhietala <[email protected]>

Update text/0000-RouteInfo-Metadata.md

82e7e81

Co-Authored-By: chadhietala <[email protected]>

Update text/0000-RouteInfo-Metadata.md

ade9f97

Co-Authored-By: chadhietala <[email protected]>

Update text/0000-RouteInfo-Metadata.md

2c3c728

Co-Authored-By: chadhietala <[email protected]>

Update text/0000-RouteInfo-Metadata.md

7aa2971

Co-Authored-By: chadhietala <[email protected]>

Update text/0000-RouteInfo-Metadata.md

ad67775

Co-Authored-By: chadhietala <[email protected]>

Update text/0000-RouteInfo-Metadata.md

42ed7fb

Co-Authored-By: chadhietala <[email protected]>

Update text/0000-RouteInfo-Metadata.md

c191ee8

Co-Authored-By: chadhietala <[email protected]>

Update text/0000-RouteInfo-Metadata.md

36795db

Co-Authored-By: chadhietala <[email protected]>

Update text/0000-RouteInfo-Metadata.md

fa93479

Co-Authored-By: chadhietala <[email protected]>

Update text/0000-RouteInfo-Metadata.md

4a56424

Co-Authored-By: chadhietala <[email protected]>

Update text/0000-RouteInfo-Metadata.md

bfed6f3

Co-Authored-By: chadhietala <[email protected]>

Update text/0000-RouteInfo-Metadata.md

2677e9d

Co-Authored-By: chadhietala <[email protected]>

Update text/0000-RouteInfo-Metadata.md

3d2630b

Co-Authored-By: chadhietala <[email protected]>

Update text/0000-RouteInfo-Metadata.md

64b0f5a

Co-Authored-By: chadhietala <[email protected]>

Update text/0000-RouteInfo-Metadata.md

caa8d87

Co-Authored-By: chadhietala <[email protected]>

Update text/0000-RouteInfo-Metadata.md

4482746

Co-Authored-By: chadhietala <[email protected]>

Update text/0000-RouteInfo-Metadata.md

9124f7b

Co-Authored-By: chadhietala <[email protected]>

Update text/0000-RouteInfo-Metadata.md

653fd9c

Co-Authored-By: chadhietala <[email protected]>

Update text/0000-RouteInfo-Metadata.md

17b6ee2

Co-Authored-By: chadhietala <[email protected]>

tylerturdenpants mentioned this pull request Nov 8, 2018

The Ember Times - Issue No. 72 emberjs/website#3671

Merged

12 tasks

chadhietala added the Final Comment Period label Nov 14, 2018

ro0gr reviewed Nov 14, 2018

View reviewed changes

MelSumner mentioned this pull request Nov 28, 2018

Octane Tracking Issue emberjs/ember.js#17234

Closed

chadhietala merged commit 2903498 into master Nov 30, 2018

jacobq reviewed Dec 5, 2018

View reviewed changes

chadhietala mentioned this pull request Dec 5, 2018

[FEATURE] buildRouteInfoMetadata tildeio/router.js#275

Merged

rwjblue deleted the route-info-meta-data branch January 23, 2019 16:38

kategengler assigned chadhietala Feb 12, 2019

kategengler mentioned this pull request Feb 12, 2019

RFC #0398 - Tracking for RouteInfo Metadata emberjs/rfc-tracking#10

Closed

2 tasks

ryanto mentioned this pull request Jun 28, 2019

Bump ember-source from 3.4.8 to 3.11.0 embermap/ember-data-storefront#106

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RouteInfo Metadata #398

RouteInfo Metadata #398

chadhietala commented Nov 2, 2018

Leooo commented Nov 9, 2018 •

edited

Loading

chadhietala commented Nov 14, 2018

lolmaus commented Nov 14, 2018

Leooo commented Nov 14, 2018

chadhietala commented Nov 14, 2018

lolmaus commented Nov 14, 2018

ro0gr Nov 14, 2018

chadhietala Nov 16, 2018 •

edited

Loading

ef4 Nov 16, 2018

ef4 Nov 16, 2018

ef4 Nov 16, 2018

chadhietala Nov 16, 2018

chadhietala commented Nov 19, 2018 •

edited

Loading

ro0gr commented Nov 19, 2018 •

edited

Loading

chadhietala commented Nov 20, 2018

jacobq Dec 5, 2018


		While the `RouteInfo` object is sufficient in providing developers metadata about the `Route` itself, it is not sufficient in layering on application specific metadata about the `Route`. This metadata could be anything from a more domain-specific name for a `Route`, e.g. `profile_page` vs `profile.index`, all the way to providing contextual data when the `Route` was visited.

		This metadata could be used for more pratical things like updating the `document.title`.

RouteInfo Metadata #398

RouteInfo Metadata #398

Conversation

chadhietala commented Nov 2, 2018

Leooo commented Nov 9, 2018 • edited Loading

chadhietala commented Nov 14, 2018

lolmaus commented Nov 14, 2018

Leooo commented Nov 14, 2018

chadhietala commented Nov 14, 2018

lolmaus commented Nov 14, 2018

ro0gr Nov 14, 2018

Choose a reason for hiding this comment

chadhietala Nov 16, 2018 • edited Loading

Choose a reason for hiding this comment

ef4 Nov 16, 2018

Choose a reason for hiding this comment

ef4 Nov 16, 2018

Choose a reason for hiding this comment

ef4 Nov 16, 2018

Choose a reason for hiding this comment

chadhietala Nov 16, 2018

Choose a reason for hiding this comment

chadhietala commented Nov 19, 2018 • edited Loading

ro0gr commented Nov 19, 2018 • edited Loading

chadhietala commented Nov 20, 2018

jacobq Dec 5, 2018

Choose a reason for hiding this comment

Leooo commented Nov 9, 2018 •

edited

Loading

chadhietala Nov 16, 2018 •

edited

Loading

chadhietala commented Nov 19, 2018 •

edited

Loading

ro0gr commented Nov 19, 2018 •

edited

Loading