Skip to content
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 81 additions & 0 deletions 0000-always-run-model-hook.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
- Start Date: 2017-12-07
- RFC PR: (leave this empty)
- Ember Issue: (leave this empty)

# Summary

The model hook will always run, even for dynamic segments that already have their models loaded.

# Motivation

Currently the router will not run the model hook when transition to a route with a dynamic segment that already has its model loaded. This behavior is well intended, if we are using `{{link-to "post.show" post}}` then there's no reason to run the `post.show` model hook, the model has already been loaded!

The guides do a great job [explaining](https://guides.emberjs.com/v2.17.0/routing/specifying-a-routes-model/#toc_dynamic-models) this feature.

Before I started using JSON:API I thought this feature made a lot of sense, it would only fetch models when the application didn't have them. However, over the last year there's been a pattern I've seen in a couple Ember applications that this behavior complicates.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this isn't that much related to JSON:API spec but more to client-side caching, which is provided by Ember Data out of the box.


Let's say we have a `posts.index` route that lists all of the posts. It's model hook:

```js
// app/routes/posts/index.js

export default Ember.Route.extend({
model() {
return this.store.findAll('post');
}
});
```

And then we also have a `posts.show` route that shows a specific post. When viewing a post we want to side load some additional data, like the post's comments.

```js
// app/routes/posts/show.js

export default Ember.Route.extend({
model(params) {
return this.store.findRecord('post', params.post_id, {
include: ['comments.author'],
reload: true
});
}
});
```

This creates an issue: Since the post model is already loaded, when we click a link on the index page to view a post, the model hook never runs. This causes the post to either render without comments, or worse, render with an async call to comments and then N+1 async calls to those comment's authors.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not that much an issue with the record being loaded but being skipped due to passing the record and not it's ID to <LinkTo> it transitionTo. To be more precise the issue that another place in the application controls if the model hook run. This is very error prune. The model hook itself should control if it reruns.


To code around this behavior a popular pattern I've seen is to link to the post ID. Since the ID is not the model, Ember will run the model hook and the correct data will be loaded.
Copy link
Contributor

Choose a reason for hiding this comment

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

A better solution (if you really take the time to understand the problem) is to fetch the model in the model hook and fetch additional data in the afterModel hook.

I think passing the id is usually not the best solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or use async model() {} and fetch the additional data in the model hook, especially if you need to use that data to load the primary model.

Copy link

Choose a reason for hiding this comment

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

Also your model could use the links attribute as I do ofen. So all comments or related data will fetch single request.

    links: 
        comments:  /posts/:id/comments

using hook every time every transition put your app slow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Passing the state which is serialized in the URL and use client-side caching in the model hook (e.g. through Ember Data) is the correct solution in my opinion. The code path if entering the route directly or through a transition within the app should be the same to reduce complexity, testing cases and possible bugs.

Copy link
Contributor

Choose a reason for hiding this comment

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

A second example is transitionTo. A child route back transitionTo back the parent route will not fire the model hook. This requires one to include all the data necessary in the options, say, for the setupController and any components in the tree to not hard fail b/c the data isn't there.


```hbs
{{link-to "posts.show" post.id}}
```

However, this isn't intuitive. From looking at this `link-to` it's not clear why ID is being used.

As an aside, I posted [this poll](https://twitter.com/ryantotweets/status/938446826117746689) on Twitter and from the answers it appears that other folks find this confusing or use the ID work around. The Ember crowd on Twitter represents folks who are in-tune with the framework, if this many experts feel this way I think it's a sign we can improve ergonomics.

# Detailed design

We would introduce a new route base class which would default to always running the model hook. In order to avoid confusion, this new route class would have a new hook called `findModel` that would always run.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be overly complex in my opinion. Did you considered deprecating passing non-string values as model to <LinkTo> or transitionTo? This would also ensure that model hook runs always but be less complex. It would also ensure that the state can be serialized to the URL.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another example here is where a modal is /foo/new and when the modal closes you need /foo model hook to refire. Right now that use case is fraught with duplication or errors (esp when the model hooks is quick packed with logic)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh RouterService.refresh. I have an opt-out for this specific scenario.


In addition to always running the `findModel` hook when transitioning, this new route class would:

* Return a POJO, allowing you to pass arbitrary names to the template. Template rendering will wait for all promises in the POJO to fulfill.
* Parent/child routes would not waterfall. Child routes would fire their `findModel` hooks before parent routes have resolved. This would cause `modelFor` to return a promise.

The new route base class would allow us to implement these new ideas without worrying about the API of the existing route class. Developers can swap out their route classes when they are ready to adapt this new behavior.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this RFC should not deal with these topics. I don't see how they are related.


# Drawbacks

If applications are currently linking to string IDs how do we pass them through the route's `serialize` function.

# Alternatives

### Always run model hook

Always run the model hook on the current route class when entering a route. The params arguments would be generated from the route's serialize function if passed a model or object.

The model hook can then make a decision if it needs to refetch the model. This seems to be the approach Ember-data has taken with `findRecord`, it's aware if the model has already been loaded, and if it is it will do a background reload. This leaves the data fetching decision up to the model layer.

# Unresolved questions

I'd love to make an addon that can proof-of-concepts this. I would need some help though.