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

Calling ".refresh()" from the router service #592

Closed
NullVoxPopuli opened this issue Feb 8, 2020 · 10 comments
Closed

Calling ".refresh()" from the router service #592

NullVoxPopuli opened this issue Feb 8, 2020 · 10 comments

Comments

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Feb 8, 2020

Right now, refresh only exists on the route.
It'd be fantastic if we had a central place to call refresh.

Update 1

@service router;
// ...
// refreshes current route
this.router.refresh() 
// refreshes the application and all child routes
this.router.refresh('application') 

// maybe typed this way
refresh(routeName?: string): Promise<void>;

Original Text

Maybe this is something that exists on each RouteInfo that makes of the list of route infos representing the current route?

Given:

this.route('posts', function() {
  this.route('show', { path: ':id' });
})

And I want to refresh just the show route, maybe I'd do:

@service router;

// ...
this.router.currentRoute.refresh();

And give that the route tree is represented as a linked list:

<RouteInfo#application>
   ⇩
<RouteInfo#posts>
   ⇩
<RouteInfo#show>  <- currentRoute

maybe I want to refresh just the posts route?

this.router.currentRoute.parent.refresh();

Thoughts?

@nightire
Copy link

nightire commented Feb 9, 2020

this.router.refresh() -> refresh the current route.

this.router.refresh(ROUTE_NAME) -> refresh a specific route (along with its children of course), and if it is not in the current route tree, throw an error in dev and do nothing (maybe?) in production.

The reason that I'm not a fan of this.router.currentRoute.parent.refresh():

Let's say I have a component that can refresh a specific route, 'posts' for example, I can render this component in any level of child route of 'posts', so the length of the parent chain can not be determined easily. So why not just use the route name as we used to in the transitionTo method?

@jaydgruber
Copy link

I have questions about how this would work with queryParams. With sticky queryParams? How is this different from calling this.transitionTo(currentRouteName)?

@jakebixbyavalara
Copy link

this.router.refresh() -> refresh the current route.

this.router.refresh(ROUTE_NAME) -> refresh a specific route (along with its children of course), and if it is not in the current route tree, throw an error in dev and do nothing (maybe?) in production.

The reason that I'm not a fan of this.router.currentRoute.parent:

Let's say I have a component that can refresh a specific route, 'posts' for example, I can render this component in any level of child route of 'posts', so the length of the parent chain can not be determined easily. So why not just use the route name as we used to in the transitionTo method?

I completely agree with this ☝️. Because I see the RouteInfo class as more of a "snapshot", I think that treating the refresh method the same as this.router.transitionTo makes a bit more sense.

If we were to put the refresh method on the router service, I think that the most common use case will be this.router.refresh() with no arguments because 90% of the time you'll want to refresh the route you're currently on.

Also by allowing the use of a route name to target a specific route, you can easily make use of the currentRoute routeInfo. if you want to refresh starting up a couple parent routes, you could use this.router.refresh(this.router.currentRoute.parent.parent.name) and it will refresh the model hooks from that parent and down through all the children.

From the docs:

A RouteInfo is an object that contains metadata about a specific route within a Transition. It is read-only and internally immutable. It is also not observable, because a Transition instance is never changed after creation.

I think that putting the method on the RouteInfo class breaks from the intent of it. The find method makes sense because it's meant to make traversing the linked list of routeInfos convenient.

I have questions about how this would work with queryParams. With sticky queryParams? How is this different from calling this.transitionTo(currentRouteName)?

This would be no different than calling this.refresh() from a route, so I don't think it's much of a concern.

@jaydgruber
Copy link

Along the lines of currentRoute.parent.refresh, I started thinking about router.refresh(depth = 1)... not much better. Would be easy to get mixed up with implicit index routes too. I agree that a route name would be a lot less prone to confusion

@btecu
Copy link

btecu commented Feb 14, 2020

I've seen developers misusing Ember Data. Usually when they get to a point that things don't work anymore, the solution is to reload. This .refresh() api would only encourage that anti-pattern.

@jakebixbyavalara
Copy link

I've seen developers misusing Ember Data. Usually when they get to a point that things don't work anymore, the solution is to reload. This .refresh() api would only encourage that anti-pattern.

IMO it's better than having them resort to using ember-route-actions and/or writing route and controller boilerplate to achieve the same thing. We could put a note in the docs steering from misuse, but if they want to refresh, they'll find a way, and I'd rather it be from a concise call than through a bunch of boilerplate.

@btecu
Copy link

btecu commented Feb 14, 2020

@jakebixbyavalara What are some use cases for the .refresh() api?

@balinterdi
Copy link

balinterdi commented Feb 14, 2020

  1. If not using ember-data, it can be quite cumbersome to reload the model data for the route. If you have a list of items (songs, for example :) ) as your model and you add a new one, the list will not re-render and you have to resort to manually setting the model to the new array, or route actions.
  2. Even if you use ember-data, your model hook needs to return a live-updating collection (is that the name?) for the newly created record to appear on the page. I think the more complex your application is, the less likely it is that you get away with a simple store.findAll. (I know there's query.reload but I think having router.refresh would be a more consistent API.)
  3. If you want to periodically check if the back-end has new data that you might to show on the page. That's quite similar to 1. and it's probably simpler to implement without router.refresh because you can set up the polling mechanism in the route and call this.refresh but it's still a separate use case.

Of course, I'll let @jakebixbyavalara add other use cases, but I also feel that this is just an omission and that the router service (which is already an extremely powerful piece of Ember's architecture) should get that "power".

@deanpapastrat
Copy link

Just want to pitch in and say that I agree with it being cumbersome to reload non-Ember Data routes using just closure actions, and it often requires passing actions very deep in the tree, which is not great when you have to refactor. An addition to the router service seems very logical to me.

@mixonic
Copy link
Member

mixonic commented Dec 3, 2020

Addressed by #631

@mixonic mixonic closed this as completed Dec 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants