rfcs
rfcs copied to clipboard
emberjs
Always run model hook
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 model hook, I would argue we should also make it return a POJO so you can pass arbitrary names into your templates (instead of just "model").
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 modelFor so that it returns a promise, which would let you block on your parent route only when you really need to. It also probably implies an option for parent routes to force their children to wait, so they can implement things like authorization checks.
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:
- Add a new hook to
Route(sayfindModel()) that implements the new behavior - Add a static config variable that changes the behavior of the existing
model()hook - Add a runtime flag to
Routeclass that changes the behavior ofmodel() - Others?
Also, can this change be made exclusively within the Route layer/class (and not require changes to links or calls to transitionTo)?
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 {{link-to}} or not). Darned if I can find that discussion now... Either way, I think the router needs a little TLC to improve on these scenarios.
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.
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:
serviceUsedJustToPassTheModelAround: service(),
model(params) {
let myModel = this.get('serviceUsedJustToPassTheModelAround.myModel');
if (myModel) {
return myModel;
}
return ...;
}
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 /post route with the model being a singe post. If only we could live in a world where the model for a /post/:id route was just the post. I have found this to almost never be the case. Looking through the model hooks in the reasonably large app I am working on, they almost always return a more complex set of data. The afterModel hook is not as useful as it does not have direct access to the params.
My favorite part of the RFC:
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. I like this thinking because the it leaves the data fetching decision up to the model layer.
^ 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.
Truly, it is not the responsibility of the linker to determine the data needed for the linked route.
Well said @Duder-onomy!
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.
@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.
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.)
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 afterModel hook is not as useful as it does not have direct access to the params.
The afterModel hook has the transition object as the second argument, which has the params on it.
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 serialize and model hooks, with serialize generating the params for model hook. Let's add a new hook, passedModel , which has signature (model, params, transition) and returns a promise. If provided, it is run instead of model. To get today's behavior, simply return model; in that hook. Retaining today's behavior, either for backward compatibility or for performance becomes easy while new users get the simplicity of a model hook that is run unless they don't want it to be.
@Gaurav0 it seems like passedModel would do one of two things...
// 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 alwaysRunModelHook: true|false on the route?
Actually, I was thinking simply not supplying passedModel could just run the model hook, to make it easy for everyone.
As to what passedModel could do if it was unconstrained? Something like this:
model({ id }) {
return this.findRecord('post', id, { include: 'comments' });
},
passedModel(model) {
return model.get('comments').then(() => model);
}
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? That way a lot of apps wouldn't need any code changes to benefit from this.
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 link-to or transitionTo in order to make things work. Passing the model seems to work, so they commit. In code review its a 3 character diff, and is easily lost in the context of the rest of the diff. In short, we have had many times where our app has broken because of this behavior. As a result, we never use it, and ALWAYS pass in just the ID.
Regarding the caching comment (https://github.com/emberjs/rfcs/pull/283#issuecomment-351580608), if the story there is that complicated, improvements should be made to ember-data to make the caching / peekRecord story better. Maybe findRecordIfUncached? The point being, it's not the routers responsibility to make caching work.
The point being, it's not the routers responsibility to make caching work.
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 https://github.com/emberjs/rfcs/issues/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.
Example
My test verifies that clicking on {{link to "Awesome Page" model}} works, but my users are clicking on {{link to "Awesome Page" model.id}} and the app breaks.
Related and also confusing subtlety: A route with no model hook, and a snake_case dynamic segment like :user_id will result in the store trying to fetch a record. Saving 1-2 lines of code is not worth the confusion around nuance and unexpected behavior.
I think we have strong consensus on the goal here.
@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.
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 ...
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:
We need a more detailed plan for how to roll this out in a compatible way.
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.
@ef4 Would shipping an add-on with a new Route class that started moving us towards behaviors we are looking to adopt work here? Including the below rfc 👇.
https://github.com/emberjs/rfcs/pull/774
I don't think this requires an RFC. Buyer beware if you decide to use it. Just curious on your thoughts overall.
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.