Uh oh!
There was an error while loading. Please reload this page.
unsubscribe some subscriptions on destroy / first some subscriptions - #1549
Conversation
jelbourn
left a comment
There was a problem hiding this comment.
Looks like some unit tests are failing as well.
| QueryList | ||
| } from '@angular/core'; | ||
| import { Subscription } from 'rxjs/Subscription'; |
There was a problem hiding this comment.
Remove spaces from around imported symbol
| public destroy() { | ||
| this._changesSubscription.unsubscribe(); | ||
| } |
There was a problem hiding this comment.
This particular unsubscription isn't necessary because the QueryList will be destroyed at the same time as the component (and the subscription is contained within the component that also contains this LineSetter).
| ngAfterContentInit() { | ||
| // On changes, assert on consistency. | ||
| this._sidenavs.changes.subscribe(() => this._validateDrawers()); | ||
| this._sidenavChangesSubscription = this._sidenavs.changes |
There was a problem hiding this comment.
This one also shouldn't be necessary because the QueryList is destroyed when the component is destroyed
fxck
commented
Oct 20, 2016
unscubscribe some subscriptions on destroy / first some subscriptionsunsubscribe some subscriptions on destroy / first some subscriptionsjelbourn
commented
Oct 20, 2016
@fxck |
@jelbourn I know this is a rather naive test http://plnkr.co/edit/vDeFQunSeXehUssR1Pzr?p=preview but both ViewChildren and Output / EventEmitter seems to still have observers on them even after the component is destroyed.. But it could be something due to caching, since it still got the same amount of observers after toggling the component back in. |
jelbourn
commented
Oct 20, 2016
@fxck they still exist in that case because you're saving references to them; if nothing references the emitter, it will be garbage collected. |
Alright then, I have removed everything but those few |
jelbourn
commented
Oct 20, 2016
LGTM |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
I'm not 100% sure you need to unsubscribe those
changessubscriptions, but I don't see it being done automatically anywhere(https://github.com/angular/angular/blob/2.1.0/modules/%40angular/core/src/linker/query_list.ts#L11-L100).Also I'm not 100% sure that dialog and icon needs that
first, but looking at code I think both should have it.There's also a couple of subscriptions on EventEmitter(in sidenav particularly) which I haven't touched since I supposed those are automatically unsubscribed.
Also I haven't touched the one mentioned in #1534
@jelbourn
// edit
looks like
changesis event emitter as well https://github.com/angular/angular/blob/master/modules/%40angular/facade/src/async.ts so I guess either it's not needed to unsubscribed at all, or those event emitter subscriptions in sidenav needs to be unsubscribed as well