$stateChangeSuccess is fired before previous state $scope is destroyed in 0.2.16

[bighuggies]

This fiddle shows the difference in order of $destroy and $stateChangeSuccess between 0.2.15 and 0.2.16

https://jsfiddle.net/n3emkeo7/

This change broke our application, and I think it could be quite a significant breaking change.

My guess is that it's something to do with 1be1379 (unconfirmed).

Our use case:

We have a navigation control which listens for $stateChangeSuccess and subsequently updates the URL using $state.go.

In 0.2.15 this was fine if you navigated to a different page, the navigation control was destroyed before the $stateChangeSuccess event was fired, so the control would only redirect the user if they were navigating to a child state.

In 0.2.16 the $stateChangeSuccess event is fired before the control is destroyed, causing the user to be trapped on the page with the navigation control regardless of if they are trying to navigate away from the page.

[nateabele]

Yeah, destroying scopes is necessarily deferred by the $animate API, otherwise views behave unpredictably when transitioning out. Scope management is nondeterministic relative to transition events, which is why we never make any guarantees about ordering.

I'm not sure there's anything we can do here, but I'm open to suggestions...

[bighuggies]

I think a change in the order of events should be considered a breaking change and should be highlighted in the changelog. It's very easy to accidentally depend on the order of events without realising you're doing it. Highlighting it in the changelog will let people know to look out for cases where they do this.

Semantically I think it makes more sense for $destroy to trigger before $stateChangeSuccess. I have no idea how difficult that is to achieve so I don't expect the behaviour to revert. It's something of an edge case so unless a lot of people come forward with issues it's probably fine to leave it.

I'll look into writing a test to verify the order of events firing (if one doesn't exist already) so we know if it changes in the future.

[limax84]

+1
This change broke our applicationS as well. On state changes, new controllers are now instantiated BEFORE old ones are destroyed. Does anyone know if this behavior is intended to be or if it is a side effect of some other changes ?
We will have to stick with the 0.2.15 version... :(

[dsendner]

+1 we had the same issue as well as we were relying on the $destroy event to reset some services when the view change

[cappsool]

+1 same issue for us as well

[nateabele]

Hi guys, sorry for your trouble. Again, this was the result of a fix for people with animated view transitions, where controller scopes were getting destroyed too early, causing rendering issues during animation.

@christopherthielen I guess we could try to detect whether people are using animation and opt them into the old behavior...? :-/

[ghost]

+1 Change it back. :(

[tempoc]

This also introduces ambiguity in terms of the "current" state and its properties. We got a bug where params that used to be there at the time of navigation are no longer available.

I understand the need for the original fix, but this is definitely a breaking change.

[willhighland]

+1 we are also having issues because of this change

[nateabele]

@4Ants I don't think we can change it back, sorry.

I don't have time to look at it right now, but I'm open to suggestions on how we could improve it without reintroducing broken behavior.

[chevcast]

This change breaks our angular-spinners library which relied on the $destroy event to unregister any disposed spinner directives from our service. The spinner tracking service can only have one spinner with the same ID at a time so when views transition the previous spinner needs to be unregistered first so the new one can register its new scope with the spinner tracking service under the same ID.

If we can no longer depend on this $destroy event is there some other event or way to hook into this so the spinner directive's scope is properly unregistered before the new directive tries to register itself with the service?

Edit: We decided to change our behavior so that new spinners with the same name override old spinners registered with that name. It's not ideal but it's better than making all users of both angular-spinners and ui-router key a manual unregister call from $stateChangeStart.

[ghost]

This is a breaking change.
Can this be introduced in major version or applied only if animation is used?

[ggoodman]

I've also had my code broken but appreciate the work that went into the last releases. @nateabele and @christopherthielen do you have any suggestions on how we might refactor our code so that we are able to 'destroy before create'?

[christopherthielen]

To those affected by this:

The ui-router philosophy is that the state machine drives the application. The views are secondary things that happen in response to the state changing. They're somewhat non-deterministic (you could pop them in and out of existence with an ng-if, for instance). I consider the views almost as side effects.

See if you can refactor your code to use the state machine hooks and events instead (onExit, $stateChangeSuccess) to perform your exiting cleanup.

Otherwise:

for the time, please revert to 0.2.15; We'll try to come up with a solution and plan a 0.2.18 (le sigh...)

---

I'm open to suggestions on how we could improve it without reintroducing broken behavior.

I'll echo @nateabele 's sentiment. Let us know if you have any ideas. Consider the issue that this change was addressing: #1643 where the view being animated out had its scope destroyed, and thus the outgoing view was mangled.

Have a look at this code:
https://github.com/angular-ui/ui-router/blob/master/src/viewDirective.js#L135-L172
https://github.com/angular-ui/ui-router/blob/master/src/viewDirective.js#L192-L224

We could really use the help right now. There are a lot of proverbial balls in the air right now, and it's tough to keep up.

[christopherthielen]

I created PR #2497 which selects the "static" (synchronous) renderer in the following cases:

• $animate.enabled returns false (this is the case when the app does not use ngAnimate)
• the attribute noanimation is present on the ui-vieww, i.e, <div ui-view noanimation></div>

Try this version which has my code from pr #2497: https://rawgit.com/christopherthielen/842ee74c30299093aa07/raw/200f188394d5b05de68e092e6b5c60905671e3ca/angular-ui-router.0.2.18-pr.js

Here is @bighuggies fiddle with pr #2497 http://plnkr.co/edit/fpE6CBzRN7zCGFng3bcb?p=preview tested against 1.2.28 through 1.5.0-rc01 with and without ngAnimate.

[christopherthielen]

CC: @bighuggies @limax84 @DDay34 @cappsool @4Ants @willhighland @tempoc @chevex @ggoodman

[willhighland]

@christopherthielen I've tested your PR against our app using the noanimation attribute on ui-view and it works as expected. $destroy events are fired on the previous controller before the new controller is created on state changes.

So at least for our use case, this does seems to fix the issues we were having. Thanks for the quick response!

[ledhed2222]

Out of curiosity, I'm not quite sure how I WOULD refactor my code to better initialize controllers. The 'resolve' element works great for passing data into a controller, but I'm not sure how to run initialization procedures when switching between child states, without resorting to $stateChangeSuccess.

@christopherthielen or @nateabele, what is the ideal way to handle initialization of a controller upon state changes that share the same controller?

[christopherthielen]

@ledhed2222 Can you be more specific?

In general, my controllers:

• accept injected data and services
• place the data on the scope (or this) for the view to bind to
• provide some functions on the scope (or this) for the view to invoke, which generally pass through to the services.

I don't find the need to manage lifecycle in the controller beyond what I listed above.

What is your controller doing that you need to handle scope $destroy events, for instance?

[ledhed2222]

Thanks @christopherthielen

For one, I'm quite new to Angular and this lib. I've inherited some code that, based on your comment above, might not have been written ideally. Here's the general idea:

Parent state A (should be abstract, but wasn't implemented that way)
Child states B and C

All of these states share the same controller X. X needs to re-initialize some scope variables and call functions defined within the controller whenever the view moves between states B and C. As I write this, I suppose it could all be refactored with a big 'resolve' dictionary, injecting everything into X. Is that the pattern?

[christopherthielen]

When the view moves between states B and C, B is exited and C is entered. A new instance of the controller X is instantiated/injected within the context of state C and C's resolves.

Are you concerned about the second copy of the controller X that is active within the context of parent state A? (state A is retained; it is neither exited nor reentered)

[ledhed2222]

Hmm, I guess that's not what I'm seeing - switching between the two child states doesn't seem to instantiate a new controller but rather uses the same instance. Now, I didn't realize that a parent state's controller instance will persist (or even that the parent's controller would be instantiated if we're in a child); could that have to do with this issue?

Also, this is probably getting off topic of the main issue here. If there's a better place for me to continue this discussion please let me know. And thank you!

[christopherthielen]

since nobody else has commented about the PR in a week, I'm pulling the trigger

[kenjiqq]

I understand the need for this change. But would it not make sense for the $stateChangeSuccess event to trigger after the previous state is destroyed, as the state is not fully transitioned yet before that?

On another side not there are other issues with this change as well, such as a states $stateParams changing because you are on another state before it is destroyed

[kenjiqq]

Also would it be possible to have a config on uirouter to disable this? Having to set noanimate on all uiviews in the app if you don't want to disable all ngAnimations are very brittle.

[christopherthielen]

But would it not make sense for the $stateChangeSuccess event to trigger after the previous state is destroyed, as the state is not fully transitioned yet before that?

Be careful not to conflate views with states. In ui-router, the transition is complete when the states are all entered and the $stateChangeSuccess event is fired. In response to the $stateChangeSuccess event, all ui-views in the DOM update themselves, according to the view configs from the target state (and ancestor states) from the successful transition.

States don't get destroyed, only scopes do. Scopes get destroyed when their DOM is destroyed (basically) or in the case of animation, when the ui-view is animating out because it is being replaced.

a states $stateParams changing because you are on another state before it is destroyed

Can you provide more details? The $stateParams injected into a controller (filtered to the params for that state) should be a different object than the shared global service $stateParams (this is undoubtedly confusing). I don't believe the $stateParams that the controller gets is ever mutated, while the global service variable is mutated.

Also would it be possible to have a config on uirouter to disable this?

possibly...

[kenjiqq]

Can you provide more details? The $stateParams injected into a controller (filtered to the params for that state) should be a different object than the shared global service $stateParams (this is undoubtedly confusing). I don't believe the $stateParams that the controller gets is ever mutated, while the global service variable is mutated.

The $stateParams object injected into controllers are definitely mutated. We watch it for changes to the params to update the page running if the url is changed.

This actually caused most of the issues we had after this change was introduced.
We would have code that watched $stateParams.someParam and if that param changed to undefined we update the url by adding this param with some default value(usually to reflect a choice in a select field or a tab on that page).
This code would now trigger after a stateChange to a different page and the code would add the param back to the url since it was removed and cause another state change.

(Previously these watches were removed before the the ui-view transitioned because the scope was destroyed)

[kenjiqq]

If anyone needs it i made this little directive to globally force the old way to render on all ui-views

angular.module('ui-view.hotfix', [])

.directive('uiView', function () {
    return {
        priority: 410,
        compile: function (tElement, tAttr) {
            tElement.attr('noanimation', true);
            tAttr.noanimation = true;
        }
    };
});

[christopherthielen]

@kenborge nice.

@nateabele and I are reconsidering the changes merged in via #2346 attempting to fix #1643. stay tuned.