Skip to content

Commit

Permalink
fix(content): unsubscribe from viewCtrl observables after content ins… (
Browse files Browse the repository at this point in the history
#10050)

* fix(content): unsubscribe from observables on destroy

* fix(content): scroll is initialized before subscribing

fixes #9593
fixes #10045

* fix(content): unset viewCtrl subscribers on destroy
  • Loading branch information
cmorbitzer authored and manucorporat committed Jan 18, 2017
1 parent fba1596 commit 3a718de
Showing 1 changed file with 14 additions and 12 deletions.
26 changes: 14 additions & 12 deletions src/components/content/content.ts
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,10 @@ export class Content extends Ion implements OnDestroy, OnInit {
_fixedEle: HTMLElement;
/** @internal */
_imgs: Img[] = [];
/** @internal */
_viewCtrlReadSub: any;
/** @internal */
_viewCtrlWriteSub: any;

private _imgReqBfr: number;
private _imgRndBfr: number;
Expand Down Expand Up @@ -328,33 +332,28 @@ export class Content extends Ion implements OnDestroy, OnInit {
this._imgReqBfr = config.getNumber('imgRequestBuffer', 1400);
this._imgRndBfr = config.getNumber('imgRenderBuffer', 400);
this._imgVelMax = config.getNumber('imgVelocityMax', 3);
this._scroll = new ScrollView(_plt, _dom);

if (viewCtrl) {
// content has a view controller
viewCtrl._setIONContent(this);
viewCtrl._setIONContentRef(elementRef);

var readSub = viewCtrl.readReady.subscribe(() => {
readSub.unsubscribe();
this._viewCtrlReadSub = viewCtrl.readReady.subscribe(() => {

This comment has been minimized.

Copy link
@masimplo

masimplo Jan 18, 2017

Contributor

Wouldn't it be cleaner if you did:

viewCtrl.readReady
    .take(1)
    .subscribe(() => {
     this._readDimensions();
});

Instead of unsubscribing inside the subscribe handler?
Also for ngDestroy you can use takeUntil instead of unsubscribing as Ben Lesh suggests:

private _componentDestroy: Subject<any> = new Subject<any>();

viewCtrl.readReady
    .takeUntil(this._componentDestroy)
    .take(1)
    .subscribe(() => {
     this._readDimensions();
});

public ngDestroy(){
     this._componentDestroy.next(true);
}

This comment has been minimized.

Copy link
@manucorporat

manucorporat Jan 18, 2017

Contributor

@masimplo hey! love the idea. I am a noob with the Observable API. would you mind opening a PR doing this the right way?

This comment has been minimized.

Copy link
@masimplo

masimplo Jan 19, 2017

Contributor

Sure thing. Here you go #10098

this._viewCtrlReadSub.unsubscribe();
this._readDimensions();
});

var writeSub = viewCtrl.writeReady.subscribe(() => {
writeSub.unsubscribe();
this._viewCtrlWriteSub = viewCtrl.writeReady.subscribe(() => {
this._viewCtrlWriteSub.unsubscribe();
this._writeDimensions();
});

} else {
// content does not have a view controller
_dom.read(() => {
this._readDimensions();
});
_dom.write(() => {
this._writeDimensions();
});
_dom.read(this._readDimensions.bind(this));
_dom.write(this._writeDimensions.bind(this));
}

this._scroll = new ScrollView(_plt, _dom);
}

/**
Expand Down Expand Up @@ -400,6 +399,9 @@ export class Content extends Ion implements OnDestroy, OnInit {
*/
ngOnDestroy() {
this._scLsn && this._scLsn();
this._viewCtrlReadSub && this._viewCtrlReadSub.unsubscribe();
this._viewCtrlWriteSub && this._viewCtrlWriteSub.unsubscribe();
this._viewCtrlReadSub = this._viewCtrlWriteSub = null;
this._scroll && this._scroll.destroy();
this._scrollEle = this._fixedEle = this._footerEle = this._scLsn = this._scroll = null;
}
Expand Down

0 comments on commit 3a718de

Please sign in to comment.