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
Always run model hook #283
Conversation
Strongly agree with the goal. We need a more detailed plan for how to roll this out in a compatible way. My assumption has been that we should use a new Route base class to switch to the new behavior. And as long as we are doing that, we should just drop most of the other complexity on Route. If it turns out we still want to reintroduce some features, that's fine to do later, whereas deciding to drop more features later is more disruptive. As long as we're introducing a new kind of And I would argue that we should change the timing so that child routes can start loading data before their parent routes have resolved. This implies changing |
I like everything you said, but that's a lot of stuff :) With today's router is it possible for an addon to create a new Route base class where we can slowly start to add these features? |
Could someone with broad knowledge of Ember's API changes chime in on the pros/cons of different approaches here? I can think of a few ways to accomplish this:
Also, can this change be made exclusively within the |
I thought @stefanpenner had some ideas for this some time ago. Basically there was a new hook that would work how @ef4 describes it, going from child back up the tree and always fires (passed in object to |
I agree that this needs much more thought. I actually like what Ember does now and appreciate it. However, I totally understand that this is confusing and would like to see better ideas. |
|
||
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. | ||
|
||
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. |
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
While always running the model hook is simple to explain and such, it also disregards that not doing so when the model is loaded is a good performance optimization. Look how unwieldy this might be if you wanted to retain this optimization in the face of model hooks that always run:
|
My first time jumping in on an RFC, but I feel passionate about this one. Ya know, it seems as though everyones examples for not always running the model hook revolve around the model for a route being a single resource. Even the ember docs use a My favorite part of the RFC:
^ Even if you are not using ember data, it is dead simple to setup data layer that knows something about cacheing. Truly, it is not the responsibility of the linker to determine the data needed for the linked route. |
Well said @Duder-onomy!
@Gaurav0 It is quite telling that this performance optimization works when you click a link with a model but breaks when using the back/forward buttons for the same route. To me this is a clear sign that the design could be improved upon. If the route's model hook were responsible for either fetching new data or returning cached data, and the model hook always ran, no matter how you got to a route you would get the performance optimization for free. (Also you don't need a new service to write this, Ember Data works just fine here.) Cookie cutter Ember apps built with best practices from the guides shouldn't behave differently when navigation is done via clicking versus using the forward/back buttons. |
I agree, that the design could use serious improvement. It is very confusing. I just don't think always running the existing model hook is that solution, it is too simple and has its own issues. I haven't noticed this breaking at all when using the back / forward button? Does using Ember Data hide that issue? Is this a bug, or is it fundamental to the design? |
The https://emberjs.com/api/ember/2.17/classes/Route/methods/afterModel?anchor=afterModel |
Ok, I think I have the solution. By default, run both the |
@Gaurav0 it seems like // old behavior
passedModel(model, params, transition) {
return model;
}
// or always run the model hook
passedModel(model, params, transition) {
return this.model(params);
} I can't think of a situation where you would do something different here, so I think letting passedModel run any code is too unconstrained. What about having a property |
Actually, I was thinking simply not supplying As to what
which would cleanly handle the case of being passed a post without the comments without fetching the post again! |
I've updated the RFC proposing a new route base class that would always run its model hook. This will allow folks to switch to these new APIs when ready, without breaking their existing routes. |
Personally I would prefer a solution that would allow me to continue not using a model hook indefinitely if that is what I want to do. |
Could we consider to keep the current hook name but make this an optional feature? |
After a few months of inactivity, I'd like to talk about my experience with this part of ember: The current behavior of the model hook is, without a doubt, the worst part of ember. Node modules? A minor papercut in comparison to the massive app breaking bugs that I've had to deal with because of this, that are near-impossible to grep for. When a new developer joins our team, ember bombards them new things to learn. Oftentimes these people will try passing random things to a Regarding the caching comment (#283 (comment)), if the story there is that complicated, improvements should be made to ember-data to make the caching / peekRecord story better. Maybe |
100% agree. @ef4 could you help us out here? I'd like to figure out how we can break up your first comment into small bite-size pieces that the community can actually follow-through on. To me, the simplest one is a a new hook or a flag somewhere, that always runs the model hook, and preserves the router's existing behavior everywhere else. I think that could help a lot of folks out. Thoughts? |
@samselikoff I started a bit down this road #305 but have not figured out how to make the router.js extendable so have put it on the backburner for now. |
This subtlety also has acceptance test ramifications, where one may think they have coverage over a given screen loading properly, but problems may still exist in a skipped model hook. ExampleMy test verifies that clicking on Related and also confusing subtlety: A route with no model hook, and a |
I think we have strong consensus on the goal here.
The blocker isn't a willingness to introduce a new flag, it's that the change spans both router.js and Ember, and the interface between the two has grown convoluted as various people have hacked in features. What we really need is good old-fashioned refactoring. Progress in this area is obviously not going fast enough, and it's because too many people fear to tread near this code. I think a lot of folks hesitate to jump in, thinking "surely somebody else already knows this code better and could do it faster than me", but that is not correct. Nobody has all this code paged into their head, it has come from disparate authors without enough holistic thinking (which is why it needs refactoring in the first place). Even if your goal is simply "make it possible to cut out the entire current router and replace it with a new experimental one", you'll still rapidly run into the need for this refactoring, in order to create a clean API boundary to make that replacement possible. This refactor is also what stands in the way of implementing the rest of the router service RFC, which has some powerful primitives that remain unimplemented that would let people do very customized routing things. The good news is we have good test coverage, so a refactor that keeps the tests green at each step is highly likely to succeed, and the tests will help guide learning about why the existing code does what it does. |
Understood – thanks for taking the time to respond. We'll try to carve out some time to work on the refactor but I have no timeline as to when it will be right now. |
conceptually, is there anything the RFC needs to address? maybe a better migration path? |
As an update here, there were a number of router refactors done back in August/September 2018 that might make this easier to pursue down the road ... |
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.
I think we should move forward with this RFC. Skipping the model hook is error prune. Especially people starting with Ember running into this issue often. It's coming up both in Discord as well as in StackOverflow on a regular basis.
But I think the RFC and the discussion here is unnecessary complex. I don't see a need to introduce a new route base class. Passing only strings as models is enough to ensure that the model hook runs always.
Sadly we can't lint against passing complex values as model to <LinkTo>
and transitionTo
. It will be too difficult to determine the type of the argument passed. Otherwise I'm quiet sure a lint rule would already exists and be part of the default configuration.
But using a deprecation at framework level consumers would be able to enforce that only strings are passed. And ember core itself can be simplified as soon as next major is released.
I recommend that we explore that path. Maybe in another RFC to start fresh?
|
||
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. |
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.
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.
}); | ||
``` | ||
|
||
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. |
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.
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.
|
||
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. | ||
|
||
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. |
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.
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.
|
||
# 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. |
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.
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.
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.
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)
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.
Oh RouterService.refresh. I have an opt-out for this specific scenario.
* 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. |
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.
I think this RFC should not deal with these topics. I don't see how they are related.
I really appreciate this RFC being put up. This would have been quite an improvement and time saver if we could have seen this to completion. However, still dealing with this a few years later :(. I'm sure others are as well, sometimes forgetting this behavior exists and coming back to this. I've seen it within my own team. I would be happy to help get it across the line but I think it needs a sponsor in the rfc meetings. |
The blocker here is not lack of a champion in RFC meetings, it's that this RFC was a nice high-level sketch but missing way too many of the details. The feedback I gave in the very first comment has never been addressed:
I agree with the draft when it says a new base class is a good way to roll this out. However, the current Route class has dozens of methods and properties. What should be the disposition of each of them in a new class? Also, in order to teach this we need a good name for it and need to know where people will import it from. That's an important part of the RFC. Also, left unexplained is the precise behavior when people mix these new route classes with old route classes, in terms of the waterfall behavior and what APIs old routes will see when they ask about new routes. |
Yes but the main challenge is that there's no stable public API for the addon's Route class itself to use to integrate with the rest of the system. So people need to either be OK with the likelihood of breaking in a minor release or first establish a low-level public API (which we've done successfully in cases like component managers, modifier managers, etc). |
It looks like there is interest here but also some areas that need to be resolved before we can accept this RFC. If someone is interested in making the necessary improvements I'll do what I can to help facilitate the process. |
We're working on revamping the Ember Router as part of Polaris. Since this change is coming in the future we want to avoid making any major changes to the current router. However, what this RFC addresses is something that we definitely want to take into account. |
Based on @wagenet 's comment, we're going to FCP to close this but at the same time take this in to account with the work on the new router. |
Rendered