Skip to content
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

RouteInfo Metadata #398

Merged
merged 22 commits into from
Nov 30, 2018
Merged

RouteInfo Metadata #398

merged 22 commits into from
Nov 30, 2018

Conversation

chadhietala
Copy link
Contributor

chadhietala and others added 2 commits November 2, 2018 09:28
Co-Authored-By: rwjblue <me@rwjblue.com>
text/0000-RouteInfo-Metadata.md Outdated Show resolved Hide resolved
text/0000-RouteInfo-Metadata.md Outdated Show resolved Hide resolved
text/0000-RouteInfo-Metadata.md Show resolved Hide resolved
text/0000-RouteInfo-Metadata.md Outdated Show resolved Hide resolved
text/0000-RouteInfo-Metadata.md Outdated Show resolved Hide resolved
text/0000-RouteInfo-Metadata.md Outdated Show resolved Hide resolved
text/0000-RouteInfo-Metadata.md Outdated Show resolved Hide resolved
text/0000-RouteInfo-Metadata.md Outdated Show resolved Hide resolved
text/0000-RouteInfo-Metadata.md Outdated Show resolved Hide resolved
text/0000-RouteInfo-Metadata.md Outdated Show resolved Hide resolved
locks and others added 20 commits November 5, 2018 17:10
Co-Authored-By: chadhietala <chadhietala@gmail.com>
Co-Authored-By: chadhietala <chadhietala@gmail.com>
Co-Authored-By: chadhietala <chadhietala@gmail.com>
Co-Authored-By: chadhietala <chadhietala@gmail.com>
Co-Authored-By: chadhietala <chadhietala@gmail.com>
Co-Authored-By: chadhietala <chadhietala@gmail.com>
Co-Authored-By: chadhietala <chadhietala@gmail.com>
Co-Authored-By: chadhietala <chadhietala@gmail.com>
Co-Authored-By: chadhietala <chadhietala@gmail.com>
Co-Authored-By: chadhietala <chadhietala@gmail.com>
Co-Authored-By: chadhietala <chadhietala@gmail.com>
Co-Authored-By: chadhietala <chadhietala@gmail.com>
Co-Authored-By: chadhietala <chadhietala@gmail.com>
Co-Authored-By: chadhietala <chadhietala@gmail.com>
Co-Authored-By: chadhietala <chadhietala@gmail.com>
Co-Authored-By: chadhietala <chadhietala@gmail.com>
Co-Authored-By: chadhietala <chadhietala@gmail.com>
Co-Authored-By: chadhietala <chadhietala@gmail.com>
Co-Authored-By: chadhietala <chadhietala@gmail.com>
Co-Authored-By: chadhietala <chadhietala@gmail.com>
@Leooo
Copy link

Leooo commented Nov 9, 2018

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
Copy link
Contributor Author

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
Copy link

lolmaus commented Nov 14, 2018

Hi! Got a number of questions regarding this.

  1. Since the hook is fired before the beforeModel, how do I put model data into metadata?
  2. How do I update metadata from a controller or a service?
  3. 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.
  4. How do I put metadata into a template? The init hook approach requires wa-a-ay too much boilerplate for a simple {{metadata.pageTitle}}.
  5. 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
Copy link

Leooo commented Nov 14, 2018

@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
Copy link
Contributor Author

@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
Copy link

lolmaus commented Nov 14, 2018

@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?


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`.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@chadhietala chadhietala Nov 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
})
``

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it could receive the RouteInfoWithAttributes

@chadhietala
Copy link
Contributor Author

chadhietala commented Nov 19, 2018

@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
Copy link

ro0gr commented Nov 19, 2018

@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
Copy link
Contributor Author

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: importing as the name service but using inject below

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants