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

Add option for reloading controller when state URL parameters change #46

Closed
sqs opened this issue Mar 13, 2013 · 16 comments
Closed

Add option for reloading controller when state URL parameters change #46

sqs opened this issue Mar 13, 2013 · 16 comments

Comments

@sqs
Copy link

sqs commented Mar 13, 2013

If you have a .state('mystate', {controller: 'MyCtrl', url: '/a/{id}'}), then navigating from /a/1 to /a/2 does not trigger a controller reload. This appears to be by design (

if (locals === viewLocals) return; // nothing to do
) and is desirable in many cases. However, in some cases, I'd like a URL param change to trigger a controller reload, as it does using the AngularJS $routeProvider. (This allows controllers to perform initialization in their body instead of having to wrap their state params in watches.)

I can think of two ways to implement this: a $state.reload() function, or a reloadOnParamsChange param to state (similar to reloadOnSearch). I can submit a PR that implements one of these if you think that is the right approach. If I am approaching this wrong, please let me know.

BTW, great project! Thanks!

@jhiemer
Copy link
Contributor

jhiemer commented Mar 14, 2013

Hi,
I am searching for exact the same thing namely $state.reload(), which would be really helpful, when I have an overview with entities, and one entity is delete. Then it should reload the current view.

@ksperling
Copy link
Contributor

The controller already gets re-created when parameters change.

However the behaviour is somewhat different from $route in that

  • only search parameters that are listed in the URL pattern are treated as parameters for the state (e.g. '/a/{id}?from&to' would treat 'id', 'from', and 'to' as state parameters)
  • search parameters that are listed are treated the same as path parameters as far as reloading is concerned. I.e. reloadOnSearch=false is not supported at this time.

@ksperling
Copy link
Contributor

@sqs in the line you're referring to, 'locals' will only be the same object if the state AND parameter values for the state are the same. In all other cases the view (and controller) will be reloaded.

@sqs
Copy link
Author

sqs commented Mar 15, 2013

Got it. I'll try to come up with a jsfiddle or test case that reproduces what I was seeing.

@lmessinger
Copy link
Contributor

Just found this... I actually need the opposite: reloadOnSearch = false. this is because I need to change stuff inside the view directive and the controller when only the parameters change - but I need to keep them as before. Wondered what is the reason that its not supported? wouldnt you feel that it is wasting speed and memory?

would you recommend that we add it locally for us (for example, by

  var keep, state, locals = root.locals, toLocals = [];
  for (keep = 0, state = toPath[keep];
       state && state === fromPath[keep] && (!reloadOnSearch || equalForKeys(toParams, fromParams, state.ownParams));
       keep++, state = toPath[keep]) {
    locals = toLocals[keep] = state.locals;
  }

)

and let me join the others: GREAT PROJECT!

EDIT: i've tried that, works so far in the sense that it does not cause reload. However, best if we could add a $watch on $stateParams service to get the changes. any hints would be appreciated..
EDIT 2: preventing the reload prevents it from being entered into the history, so back button does not work for those parameter changes that didnt make it into a reload

@ksperling
Copy link
Contributor

@lmessinger what exactly are you trying to gain by not reloading the controller? Is there actually information held by the controller that you need to keep, or is it meant to be some sort of optimization?

The reason to create the controller from scratch is so that it can (either itself or via the 'resolve' property of the state or view) load data based on the current parameters. It sounds to me like managing all that by hand via watches is rather complicated and error prone. Compared to actually loading some data remotely, surely the overhead of re-creating a controller is tiny, and any caching would be much better done via a service.

$state also currently makes no arbitrary distinction between path and search parameters, so 'reloadOnSearch=false' would have to be something like 'reloadOnParameterChange=false'.

@lmessinger
Copy link
Contributor

Thanks @ksperling for both a great module and your answer. first about the
use case - I'm developing a search panel, represented as a state and
panel, and taking its data from the state parameters. the user types in the
input box the search text and that is directly examined by the search panel
and make new calls to the server

Except from performance, which might be an issue on weaker devices, the
other reason that I have is entry and exit animation.

in my system, every directive has an entry and exit animation. the plan is
to use onEntry/onExit for that. if the view directive is re-created on each
parameter change, I wouldnt be able to use these but would have to find a
different method (probably still need to do some change to ui-router
original code)

so far i've implemented the patch I've discussed above. but that does not
allow me to use stateParams at all in my controller - cannot put a
watch/observe on it. wondered if there's a way to do it...

thanks again
Lior
On Tue, Mar 19, 2013 at 9:07 PM, Karsten Sperling
notifications@github.comwrote:

@lmessinger https://github.com/lmessinger what exactly are you trying
to gain by not reloading the controller? Is there actually information held
by the controller that you need to keep, or is it meant to be some sort of
optimization?

The reason to create the controller from scratch is so that it can (either
itself or via the 'resolve' property of the state or view) load data based
on the current parameters. It sounds to me like managing all that by hand
via watches is rather complicated and error prone. Compared to actually
loading some data remotely, surely the overhead of re-creating a controller
is tiny, and any caching would be much better done via a service.

$state also currently makes no arbitrary distinction between path and
search parameters, so 'reloadOnSearch=false' would have to be something
like 'reloadOnParameterChange=false'.


Reply to this email directly or view it on GitHubhttps://github.com//issues/46#issuecomment-15153374
.

Lior Messinger
1-646-3730044
www.Lgorithms.com

@ksperling
Copy link
Contributor

@lmessinger Hmmm. We're going to have animation support via ngAnimate (#22).

If I remember correctly, reloadOnSearch=false in $routeProvider causes the entire route change to be omitted, i.e. no $routeChangeStart / $routeChangeSuccess event etc. So if we were to follow the same logic, we would just update $stateParams, but not fire any onEnter/onExit or resolve new locals or anything. It seems like this could cause all sorts of odd effects though, for example in the current code any onExit callback that gets $stateParams injected would see the parameters that were valid when the state was first entered, and any subsequent changes would be invisible -- however this could be fixed.

On the whole I'm not really convinced of the need for reload=false yet, especially for performance reasons: If performance is really an issue, we should do some profiling and see how we can make things faster without making people essentially do their own state management for parameters.

As far as your watch goes, try watching $state.params; $stateParams always retains the same object identify (it can't change to a different object because its managed by $injector), only its contents change. $state.params is actually a new object when the state or any parameters change.

@lmessinger
Copy link
Contributor

Thanks @ksperling . Not sure if ngAnimate (looks very cool!) will solve it since if, upon parameters change, the view directive (and all of its child directives) will be re-created, i suspect they will be re-animated. and we dont want that if we plan to keep the view.

Thanks for the watch advice

@jssebastian
Copy link

On the whole I'm not really convinced of the need for reload=false yet, especially for performance reasons: If performance is really an issue, we should do some profiling and see how we can make things faster without making people essentially do their own state management for parameters.

@ksperling: I don't think it is about performance, but more about ui flicker. In my experience with angular so far so long as there are no AJAX queries (which we can cache) the page displays fairly fast, but there are a lot of things that flicker.

e.g., ui-bootstrap's collapse/accordion does a little bounce, the new angular animations do their thing, my d3.js-based graphs take a moment to render, ng-show/ng-hide/ui-if sometimes starts in the wrong state and quickly flickers to the right one, etc.

This is not so bad on page load (in some cases it is even done on purpose, such as animations). But if it happens when a user is navigating within a page I think it is really bad.

An example: a table view with a next page button that updates the anchor part of the url to reflect which page is being shown (like gmail does). But every time you click next, the whole page flickers away and then appears again (including ui components independent of the current table page). Furthermore, the user loses the scrolling position and finds himself back at the top of the page.

@ksperling
Copy link
Contributor

@jssebastian any idea what's causing this flicker? One thing that I've found quite odd is that e.g. ng-view adds the 'raw' template to the visible DOM before running the compile/link on it. In theory the compile and link should finish before the JS code finishes (i.e. before the browser starts rendering), but maybe this is not always true?

@jssebastian
Copy link

@ksperling: from some more testing, things are not as bad as I assumed. Basically there seems to only be flicker if some state variable in my scope is briefly in the wrong state, which typically happens while I am loading asynchronously, so caching or keeping some state in a service should be able to fix most of these issues. If I find anything specific that I cannot fix on my side I will report it.

However, my d3.js-based graphs do take a moment to re-render, and losing scrolling position remains a problem.

@ksperling
Copy link
Contributor

@jssebastian scroll position is something we have to look at... clearly the $anchorScroll approach with it's default scroll-to-top behaviour isn't appropriate for ui-router anymore, see #110

@brennancheung
Copy link

So I don't know if there is a better way to do this but I had a similar problem. I had one view with a list of entities, and another that allows me to add entities. When I add the entity and submit it, the list was not being updated. I eventually found that I can use $rootScope.$broadcast('entitiesChanged') and then in the other controller I can do $scope.$on('entitiesChanged', function...). When I receive the entitiesChanged event I fetch the list from the RESTful endpoint again.

@ksperling
Copy link
Contributor

@brennancheung events seem like a good way of doing this sort of thing. If your sending from controller that's in a child scope of the one interested in the event you should also be able to use $scope.emit(), which should be more efficient in this case as the event would only traverse upwards, not to all other branches of the scope tree.

@nateabele
Copy link
Contributor

Moving the discussion to #125, since the complex parameter implementation will allow this.

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

7 participants