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

$onDestroy controller hook on leaving state #3043

Closed
alexkpek opened this issue Sep 27, 2016 · 21 comments
Closed

$onDestroy controller hook on leaving state #3043

alexkpek opened this issue Sep 27, 2016 · 21 comments
Labels

Comments

@alexkpek
Copy link

$onDestroy hook is not called when leaving state, but $scope.$on('$destroy') is working.

@christopherthielen
Copy link
Contributor

is it an angular 1.5 component? $onDestroy only works with components, as far as I know.

@alexkpek
Copy link
Author

Yup, it's Angular 1.5.8

@christopherthielen
Copy link
Contributor

ok, I'll need a plunker to replicate. http://bit.ly/UIR-Plunk

@teyc
Copy link

teyc commented Nov 15, 2016

@christopherthielen
Copy link
Contributor

@teyc thanks for the plunker.

The plunker doesn't contain any components. A component is created using angular.module('foo').component(... )

@teyc
Copy link

teyc commented Nov 15, 2016

I've added component to the sample, but the behavior is still the same.

Incidentally, instructions here don't work. https://ui-router.github.io/guide/ng1/route-to-component

@christopherthielen
Copy link
Contributor

christopherthielen commented Nov 15, 2016

OK, you defined a component, but never used it. Here, take a look: http://plnkr.co/edit/1sbmr00ZAWvRGkgYzeVd?p=preview

screen shot 2016-11-15 at 5 19 30 pm

Incidentally, instructions here don't work.

Those instructions require ui-router 1.0. The linked plunker is using 0.3.2 (the latest 'stable' release). Here's the same plunker, updated to 1.0 and route-to-component: http://plnkr.co/edit/YWYuYlO9WUyzz2kc7Yu3?p=preview

@teyc
Copy link

teyc commented Nov 15, 2016

Thank you Chris - works as advertised! 👍

@christopherthielen
Copy link
Contributor

Thanks for response @teyc

I think I will close this ticket since it seems to work, and no response from original reporter.

@amalitsky
Copy link

amalitsky commented Nov 25, 2016

Guys, despite the fact that life-cycle hooks are presented/documented in angularJs 1.5 docs for components/directives only, plain controllers used for state templates should not (IMO) behave much differently (for reference this is what ng-controller does - basically creates a directive passing controller to it)

Currently if controller for the template was defined through state definition, $onDestroy event on that controller won't ever be called. Even if this is expected it does affect usability of ui-router - to work around this I have to use ng-controller syntax in a template instead.

What do you think?

I find this angularJs issue thread to be related: angular/angular.js#15073

@adamreisnz
Copy link

adamreisnz commented Jan 12, 2017

I agree with the above. We are still using regular controllers with controllerAs syntax, and having issues that the $onInit is called when ui-router navigates to the route, but $onDestroy never gets called when you leave the route. We don't want to upgrade ui-router to 1.0.0 just yet.

As far as I am aware, the $onInit and $onDestroy life cycle hooks are available for all controllers, not just component controllers.

@christopherthielen can we re-open this issue for discussion?

@christopherthielen
Copy link
Contributor

christopherthielen commented Jan 12, 2017

Reopened for discussion

I'm of the opinion that ui-router shouldn't be in the business of re-implementing the built-in angular lifecycle. There are too many things to possibly get wrong. There are many different versions of angular, and there are differences between component lifecycles between them. That's not something I'd like to commit to maintaining.

If anything, I'm leaning towards removing the ui-router manual firing of $onInit.

Perhaps theres a better way to construct the template/controller that would lean on angular itself (instead of mimicing its behavior). Today we use $controller().

@adamreisnz
Copy link

adamreisnz commented Jan 12, 2017

Thanks, yes I think it's probably correct that ui-router shouldn't have to fire the lifecycle hooks manually. I didn't realise that $onInit was being fired manually.

After posting here, I've started looking into using components for my routes to see if it would mitigate the problem, but my routes aren't showing anything. Am I correct to assume that functionality is available in 0.3.2 as per #2627 (comment)? Or is this only implemented for 1.0.0?

@christopherthielen
Copy link
Contributor

@adamreisnz it's implemented in 1.0.0-alpha.1 and later.

A simple GitHub tip: look above the comment you found and find the referenced commit @christopherthielen christopherthielen closed this in 1552032 on Mar 26, 2016

Then, you can see what branches the commit is in:
screen shot 2017-01-12 at 3 07 57 pm

Regarding route-to-component, there are two things I should mention:

  1. Read the guide
  2. There is a polyfill for ui-router 0.2.x/0.3.x but it is limited in what it can do.

@adamreisnz
Copy link

Thanks, I'll look into migrating to 1.0.0

@christopherthielen
Copy link
Contributor

@amalitsky
Copy link

Why did it get closed? Using components instead of templates with controllers is not addressing this issue.

@adamreisnz
Copy link

adamreisnz commented Jan 12, 2017

Yep found that one.

Quick question re original topic. Since $onInit is fired manually in 0.3.2, can we not also fire $onDestroy manually to support component style controllers with events for the legacy ui-router? It would avoid us having to upgrade too much, all at once. This way we could keep using 0.3.2 for a while longer while slowly converting code base to use life cycle hooks, and later make the plunge to convert to components for routing.

@christopherthielen
Copy link
Contributor

If I add $onDestroy somebody will ask for $onPostLink or $onChanges. This is why I stated that I'd prefer to remove the existing $onInit() calling code. I think it was a mistake to add it in the first place.

I don't want to mimic angular core functionality because it would be a maintenance nightmare (I am not willing to undertake maintenance of code that mimics core angular functionality).

We use $controller() to create a template/controller, which unfortunately means we do not get the lifecycle hooks, AFAIK. Using ng-controller would get us the lifecycle hooks, but is not possible to use because we cannot inject locals. $controller() allows locals, but ng-controller does not.

Your two workaround today are:

  • route to component
.state('foo', {
  component: 'someDirective',
  resolve: { data: () => 'data' },
});
  • Route to directive template This can be done incrementally even using 0.3.2
.state('foo', {
  template: '<some-directive data="$resolve.data"></some-directive>',
  resolve: { data: () => 'data' },
});
  • or use $scope.$on('$destroy') in your controller instead of $onDestroy.

As it stands, I see route-to-component to be the future of angular-ui-router. If anyone can find a way to tie into core angular lifecycle hooks, I'll consider reopening. It must not be a breaking change, however.

@adamreisnz
Copy link

adamreisnz commented Jan 12, 2017

I think $onPostLink and $onChanges don't make sense, because there are no bindings in normal controllers. $onInit and $onDestroy are however, very basic life cycle hooks which I think ui-router can support very easily (enter route, exit route). Adding $onDestroy should not be a breaking change.

I understand the reluctancy to add more hooks and keep maintaining them, but if we could only add the relatively simple and basic $onDestroy and draw the line in the sand after that one, that'd be great. $onInit may have been a mistake, but it's in there now, so I think adding $onDestroy to complement it is not going to be a drastic addition?

Note that I do am keen to migrate to routing to components, but our application is quite large, and it will take some time before such a migration can be completed and fully tested. As such, if we were to be able to do it in steps, it would be a lot easier for us. Having the $onDestroy hook would greatly aid us, as we don't inject $scope's in our controllers anymore.

@adamreisnz
Copy link

FWIW, I've bitten the bullet and upgraded our app to ui router 1.0.0. This was surprisingly less painful than I had thought it would be, and component routing has now solved the need for the $onDestroy hook for us.

@amalitsky I would recommend upgrading as a path forward, it is not as involved as it may seem at first glance and most of the API is still backwards compatible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants