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

Need possibility to force transition to the state even if the parameters are the same #76

Closed
stasgrom opened this Issue Apr 9, 2013 · 55 comments

Comments

Projects
None yet
@stasgrom

stasgrom commented Apr 9, 2013

Hi,

I'm evaluating ui-router as a routing infra for our project. One of the issues I'm having at the moment is implementing refreshing the view. We have a refresh button which should reload the content of the view (because, for example, something might have changed in the server and we might get different data in UI). However trying to transition to the same state as we're in right now with the same parameters is blocked (I debugged and saw it in the code). So I would like to have some possibility/API to overcome this blocking.

The other possibilities, as I see, are worse:

  1. Reloading the state by explicitly transitioning through an interim state, the only purpose of which is to trick ui-router into thinking something changed. This works but it reloads the state with all its ancestor states and thus builds the whole UI from the scratch, instead of reloading only the leaf current state.

  2. Extracting the content of the state controller in a function and calling it from the refresh button handler explicitly. It works but looks bad from the architectural perspective, as it requires an extra step from the developer. One of the reasons I am evaluating the ui-router is because it allows to split the logic into state hierarchies directly, without introducing unnecessary functions, so I don't want to lose this advantage.

Thanks.

@jeme

This comment has been minimized.

Member

jeme commented Apr 9, 2013

In option 2 you wouldn't have to extract the assignment of content to scope from the controller, instead what you could do was to broadcast an event that would then trigger the same data refresh in your controller...

If the scopes are in parent/child relation (they are if the views are nested within each other) you can even just call broadcast on the parent scope (I assume that is where your refresh button is)... Otherwise, if they are siblings, you will have to broadcast it on the root scope.

I think in this scenario, events make sense to use in that way... Where as if it was just sharing data between scopes, a service (possibly with events) would be the direction to go.

Not saying the Force Refresh wouln't be a good addition, but it requires some thinking and discussion before implementation.

@stasgrom

This comment has been minimized.

stasgrom commented Apr 9, 2013

Hi jeme,

thank you for responding.

if I understand you correctly you suggest something like (I simplified the code) :

$stateProvider
.state('parent', {
controller: function($scope) {
$scope.onRefresh = function() {
$scope.broadcast('refresh-event');
}
}
})
.state('child', {
controller: function($scope) {
$scope.$on('refresh-event', function() {
// Refresh your data here
}
scope.broadcast('refresh-event');
}
})

This will work, but it doesn't look tidy. This inevitably creates an unnecessary code bloating pattern, in each every time you need to implement refreshing functionality for a state, you:

  1. Extract all logic to some event-handling function
  2. Broadcast the event when the Refresh button is hit
  3. Broadcast the event when you enter the state (or alternatively call the event handler directly, if you assign name to it)

Regards.

@jeme

This comment has been minimized.

Member

jeme commented Apr 9, 2013

Close, but with the alternative you mentioned:

$stateProvider
    .state('parent', {
        controller: function ($scope) {
            $scope.onRefresh = function () { $scope.broadcast('refresh-event'); }
        }
    })
    .state('child', {
        controller: function ($scope) {
            $scope.$on('refresh-event', refresh);
            function refresh() {
                //Refresh Logic
            }
            refresh();
        }
    })

And yes it adds a bit extra code... But this isn't really uncommon practice in many other areas...
As i said... I think this is the best thing you can do for now, as implementing refresh in ui-router will take time to figure out the best solution...

Obviously your welcome to have a look at it your self.

@ksperling

This comment has been minimized.

Member

ksperling commented Apr 9, 2013

I agree with @jeme, a 'refresh' or 'reload' method sounds like it could be reasonable, but I think there are a few variations to account for / decide on, so it's not a quick one line fix. In particular

  • do we reload the whole state, i.e. including all parent states or
  • do we reload just the leaf state or
  • do we allow individual views to be reloaded explicitly or
  • should we allow all of those?

If we do reload states rather than just views, should transition events be triggered, and onExit/onEnter be called? Intuitively I'd say "no" for these ones. Maybe have a separate onReload callback and event instead?

@stasgrom

This comment has been minimized.

stasgrom commented Apr 10, 2013

Hi guys. Thanks, it seems like for the time being I will go with this approach.

@lrlopez

This comment has been minimized.

lrlopez commented Apr 11, 2013

@stasgrom For the sake of completeness, if you need to send the refresh message upwards to the parent states you could use $scope.emit instead of $scope.broadcast

@ksperling I've been thinking about your proposal. What about this? We could implement a reload() method on the states whose default behavior would be reload all the state views that have a reload: true parameter and also call an onReload callback, if defined. Also, calling the method with reload([true | false], [true | false]) would propagate the reload event upwards and/or downwards into the state hierarchy respectively.

Should we find it useful, we could also define that returning false from the callback would stop propagation in that direction.

What do you think? It's simple enough to cover most cases and won't add too much complexity to the code.

@timkindberg

This comment has been minimized.

Contributor

timkindberg commented Apr 11, 2013

Wouldn't it always have to reload downward? Because if you are reloading a state that has children those child states would need to be reloaded too, I would think.

A slightly altered option (just throwing it out there) would be to have reload() only reload the state that its called on and all its descendants by default. That way you just have to decide how far up the state hierarchy you'll need to call reload().

Perhaps then its just an option of reload(true) to make it bubble upward as well.

@timkindberg

This comment has been minimized.

Contributor

timkindberg commented Apr 11, 2013

Or maybe reload(stateName, bubble). So reload("contact") would reload the "contact" state and all of its descendants.

@lrlopez

This comment has been minimized.

lrlopez commented Apr 11, 2013

Wouldn't it always have to reload downward? Because if you are reloading a state that has children those child states would need to be reloaded too, I would think.

May be you only want to reload data regarding to that particular state or view (i.e. a sorting column switch) that doesn't bother to descendants. But you're right, the most used and intuitive behavior should be propagate to the child states. Why just not adding an optional parameter that defaults to true?

Or maybe reload(stateName, bubble). So reload("contact") would reload the "contact" state and all of its descendants.

I like that. We could support both syntaxes, it should be easy. If you don't specify an stateName we could just use the current one.

@timkindberg

This comment has been minimized.

Contributor

timkindberg commented Apr 11, 2013

May be you only want to reload data regarding to that particular state or view (i.e. a sorting column switch) that doesn't bother to descendants. But you're right, the most used and intuitive behavior should be propagate to the child states. Why just not adding an optional parameter that defaults to true?

So it would be reload([stateName=null],[bubbleUp=false],[bubbleDown=true]) with first parameter being completely optional?

So then some examples:

  • state.reload() - reload state and all descendants (where reload == true). Seems like reload: true should be the default if not specified.
  • state.reload("contact") - reload "contact" state (regardless of where reload is called from) and all children.
  • state.reload(true) - reload state, all descendants and all ancestors.
  • state.reload(true, false) - reload state and all ancestors (not descendants).
  • state.reload("contact", false, false) - reload only the "contact" state.

You like this?

@lrlopez

This comment has been minimized.

lrlopez commented Apr 11, 2013

Yes! 👍 I think it covers most use cases and is intuitive enough...

@jeme

This comment has been minimized.

Member

jeme commented Apr 11, 2013

You will have to reload downwards as it is today, as a reload means that the view of that state gets wiped and reapplied and so does it's controller, so if you don't you will lose all the views under neath it and leave it in an uncertain state...

The most logical thing is to reload from top and down, and sure you could say "reload from this particular state and down"...

So it would be:

  • reload('root');
  • reload('root.child')
  • reload() -> Root or just current?...

And i would advocate for it to stay that way.

@timkindberg

This comment has been minimized.

Contributor

timkindberg commented Apr 11, 2013

Ok so then it would be reload([stateName=null],[bubbleUp=false])

And:

  • state.reload() - reload state and all descendants (where reload == true). Seems like reload: true should be the default if not specified.
  • state.reload("contact") - reload "contact" state (regardless of where reload is called from) and all children.
  • state.reload(true) - reload state, all descendants and all ancestors.
  • and technically you could do state.reload("contact", true) but that is identical to just reload(true).
@lrlopez

This comment has been minimized.

lrlopez commented Apr 11, 2013

I've got one question. Should be onExit and onEnter be called when reloading? May be we could define a default onReload behavior that calls onExit and onEnter but could be overridden.

@ksperling

This comment has been minimized.

Member

ksperling commented Apr 11, 2013

I think reload always by necessity moves downwards (because nested views will need to be recreated anyway), so I think it's just a question of where you start. It's probably most convenient if the default is to reload just the current leaf, so you'd have

  • $state.reload(stateOrName)
  • $state.reload() is short for $state.reload($state.current)
  • and maybe $state.reloadAll() as more obvious sugar for $state.reload('')

Because we'll need to re-resolve, reload is going to be asynchronous, so it's probably easiest to treat it like a transition (for example in terms of what happens when another transition is started before the reload is complete and other edge cases like that).

I think a transition needs to carry along a little descriptor object that has 'from', 'to', 'fromParams', 'toParams' and any other information for the transition like 'reload': true/false, and make this available to onEnter/onExit. That way we could do away with a separate onReload callback. This could also be easily extended to pass along other hints from $state.transitionTo(), e.g. allowing animation to be disabled via 'animate':false or overridden.

It would probably make sense to also change the onStateChange* events to receive that transition object instead of 4+ separate parameters.

@timkindberg

This comment has been minimized.

Contributor

timkindberg commented Apr 11, 2013

Makes sense. I knew you'd come in and lay down some knowledge. I like what
you propose.

@jeme

This comment has been minimized.

Member

jeme commented Apr 12, 2013

What I essentially did in my own repository was to:

                reload: function (state?) {
                    if (isDefined(state)) {
                        if (isString(state) || isObject(state)) { 
                            forceReload = toName(state);
                            //TODO: We need some name normalization OR
                            //           a set of "compare" etc methods that can ignore root.
                            if (forceReload.indexOf('root') !== 0) {
                                forceReload = 'root.' + forceReload;
                            }
                        } else if (state) {
                            forceReload = root.fullname;
                        }
                    } else {
                        forceReload = current.fullname;
                    }

                    $rootScope.$evalAsync(() => {
                        goto(currentState, currentParams, $route.current)
                    });
                }

Then during state dirty check i added:

else if (forceReload && forceReload == toAtIndex.state.fullname)
    toAtIndex.changed = true;

If that could serve as any sort of inspiration.

@simalexan

This comment has been minimized.

simalexan commented Apr 22, 2013

Hello guys, this could be a solution to various issues here on the list. Is there any progress on the issue?

@jlmakes

This comment has been minimized.

jlmakes commented May 1, 2013

@lrlopez I like your thinking on this, and agree this would make a cool addition to a future build of ui-router.

@Danita

This comment has been minimized.

Danita commented Jul 11, 2013

Any word on this? I'm stumbling on the same issue. One thing nobody mentioned with the method proposed by @jeme (trigger data refresh in your controller) is what happens when in your controller you use data provided by a resolve property on the route and that's the data which changed.

Or aren't resolves to be used that way and I'm doing it wrong?

@michaelhunziker

This comment has been minimized.

michaelhunziker commented Jul 18, 2013

+1 for the ability to reload a state!

I'm currently using the suggested workaround above using a refresh event.

@elmerbulthuis

This comment has been minimized.

elmerbulthuis commented Jul 24, 2013

+1 for flushing a state. I would love to see my resolved dependencies get unloaded (unresolved) in some way so that the next time the state is activated they will be reloaded (re-resolved).

i agree with @ksperling, no bubbeling

@jeme

This comment has been minimized.

Member

jeme commented Jul 24, 2013

@Danita That would work ones a true "Reload" is implemented (Or it should at least), but your correct that it does not work with that workaround.

So you will have to query that data directly in the controller instead, e.g. using the same service or resource as you would otherwise use in the resolve method to acquire the data. And then I would settle for doing the data acquisition in the controller only and not use resolve at all (There is bound to be some very special cases where that won't hold up, then you would have to do it both in resolve and refresh)

@Danita

This comment has been minimized.

Danita commented Jul 24, 2013

@jeme You're right. I ended doing the data acquisition both in the resolve and the controller; I couldn't afford doing it just inside the controller because I wanted the data to be ready before I change states. But this forced me to simplify the logic to get the data down to a single method call to a service, so it was better in the end.

@morgs32

This comment has been minimized.

morgs32 commented Aug 7, 2013

+1. Because I could check if the user is logged in at every state change but why not let my backend do that (faster). An error handling mechanism allows them to login again, but then I'd love to reload the state.

@johnculviner

This comment has been minimized.

johnculviner commented Sep 25, 2013

+1 This would be useful for my code right now, more explicit control is always a plus. Going to use the event route in the mean time...

@cm325

This comment has been minimized.

cm325 commented Sep 27, 2013

👍

@simeonwillbanks

This comment has been minimized.

simeonwillbanks commented Sep 27, 2013

👍

For now, going with @stasgrom's interim state idea, so resolve is only place which queries for data.

@akarelas

This comment has been minimized.

akarelas commented Oct 6, 2013

👍

@Routhinator

This comment has been minimized.

Routhinator commented Oct 6, 2013

+1

My code logic would work perfectly with this feature, but currently is broken if a user is prevented from transitioning by the security access. Can't wait for this.

@ghengeveld

This comment has been minimized.

ghengeveld commented Oct 29, 2013

+1 this will prevent having to put a copy of the resolves functions in the controller as well (in order to reload from within the controller). I'd like to have a $state.reload() method.

@timkindberg

This comment has been minimized.

Contributor

timkindberg commented Oct 29, 2013

Wow, this is a really popular issue..

@nateabele nateabele closed this in ca3b477 Oct 31, 2013

@timkindberg

This comment has been minimized.

Contributor

timkindberg commented Oct 31, 2013

Hell yeah nate!

@simalexan

This comment has been minimized.

simalexan commented Oct 31, 2013

Awesome guys!

@akarelas

This comment has been minimized.

akarelas commented Oct 31, 2013

Is it possible for <a ui-sref="statename(...)"> to reload?

@Danita

This comment has been minimized.

Danita commented Oct 31, 2013

❤️

@jzimmek

This comment has been minimized.

jzimmek commented Oct 31, 2013

Thx

paglias referenced this issue in HabitRPG/habitica Oct 31, 2013

try to limit need to refresh after party operations, @lefnire this is…
… only for parties and I got it working only for removeMember and leave party because I could not get join & create working
@ghengeveld

This comment has been minimized.

ghengeveld commented Oct 31, 2013

Maybe I'm missing something, but it seems my scope params are not updated on reload. I have several resolves, which I inject into a controller and there assign to a scope property. When using $state.reload, the scope properties are not updated and the view is not updated. I tried $scope.$watch on the injected values and $scope.$apply right after the reload, but to no avail. The resolves do fire again as expected (they return a $http promise). Perhaps reload().then() would work, but currently reload() does not have a return value.

http://plnkr.co/edit/GNr1bN

@ghengeveld

This comment has been minimized.

ghengeveld commented Nov 1, 2013

After some fiddling around I did come up with a workaround using a service and a listener: http://plnkr.co/edit/mcm8Dg?p=preview
This is still not ideal though.

@dedan

This comment has been minimized.

dedan commented Nov 4, 2013

@nateabele I was happy to see this feature implemented and did a quick experiment with it. To my surprise I found that calling $state.reload() does only re-run the resolves, but does not reload the controller function.

How does this help me with updating the values on my scope? Maybe I got the intention of your reload() function wrong, but I would have expected it to run the resolves again and then re-instantiate also the controller in order to put a potentially changed value on the scope.

    $stateProvider.state("test", {
      url: "/test",
      resolve: {
        statisticsService: function(StatisticsService){
          console.log('resolving');
          return StatisticsService.available();
        }
      },
      controller: function ($scope, $state, statisticsService) {
        $scope.StatisticsService = statisticsService;
        console.log('loading', statisticsService);

        $scope.reload = function () {
          console.log('calling reload');
          $state.reload();
        }
      }
    });
@nateabele

This comment has been minimized.

Member

nateabele commented Nov 4, 2013

@dedan Yeah, thought that was working, but missed it in the final implementation somehow. Working on tests & a refactor now.

@dedan

This comment has been minimized.

dedan commented Nov 4, 2013

@nateabele thanks for your quick reply. Looking forward to having a look at your solution

@sheerun

This comment has been minimized.

sheerun commented Nov 6, 2013

+1 on this issue

@MathieuGilet

This comment has been minimized.

MathieuGilet commented Nov 8, 2013

👍

@Guuz

This comment has been minimized.

Guuz commented Nov 26, 2013

So this works now but its not in v0.2.0? Any idea when it will be released? This would make my application code so much simpler!

@timkindberg

This comment has been minimized.

Contributor

timkindberg commented Nov 27, 2013

v0.3 should be out within the next month. Until then you can always build manually. See Developing section.

@Salmatron

This comment has been minimized.

Salmatron commented Nov 28, 2013

Oh my god.. $state.reload() does not work properly, fine. But why on earth does $state.go() care about it being the same page (and doesn' t do anything)? Isn't that a bug too?

@jeme

This comment has been minimized.

Member

jeme commented Nov 29, 2013

@SalmanPK Settle down... this is an Open Source project that people use their spare time to work on, pay them some respect...

And to answer your question, what you describe about $state.go is the intended behavior for cases where parameters remain the same.

@iobaixas

This comment has been minimized.

iobaixas commented Jan 3, 2014

Here is a little module that can be used as shim until the reload method is fixed:

angular.module('ui-router-reload-shim', [])
    .config(function($stateProvider) {
        // bouncer state
        $stateProvider.state('bounce', {
            params: ['state', 'params'],
            template: '<h4>Loading stuff...</h4>', // you can even put some loading template here, wow!
            controller: ['$state', '$stateParams', function($state, $stateParams) {
                // just redirect to caller
                $state.go(
                    $stateParams.state,
                    JSON.parse($stateParams.params)
                );
            }]
        });
    })
    .run(function ($state, $stateParams) {
        // override reload so it uses the bouncer.
        $state.reload = function() {
            this.go('bounce', {
                state: $state.current.name,
                params: JSON.stringify($stateParams) // only strings can be passed
            });
        };
    });

hope it helps someone.

@ghost ghost assigned nateabele Jan 4, 2014

@timkindberg

This comment has been minimized.

Contributor

timkindberg commented Jan 4, 2014

Gonna reopen this so we don't lose it.

@timkindberg timkindberg reopened this Jan 4, 2014

@nateabele

This comment has been minimized.

Member

nateabele commented Apr 23, 2014

Closing as we already have a bug report about controller reloads.

@nateabele nateabele closed this Apr 23, 2014

@christopherthielen christopherthielen modified the milestones: 0.3.0, 0.2.11 Aug 27, 2014

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