From a7ce31e60da9a3cabd8d98fb1c7ef3919697dabb Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Fri, 8 Sep 2017 22:32:51 +0200 Subject: [PATCH] fix(slider, drawer): unsubscribe from directionaly change subject (#6907) If a slider component will be destroyed, the `ngOnDestroy` hook calls `_directionality.change.unsubscribe()`, which means that the `unsubscribe` method is called on the `EventEmitter` instead of the actual `Subscription`. This causes the `EventEmitter` to be kind of "completed", which then means that there will be errors if `emit()` is called again (https://github.com/ReactiveX/rxjs/blob/master/src/Subject.ts#L96). Also fixes that the drawer never unsubscribes from the `_dir.change` `EventEmitter`. This means that even if the component is destroyed, whenever the directionality changes, the drawer calls `validateDrawers()`. Fixes #6892. Fixes #6903. --- src/cdk/bidi/bidi.md | 13 +++++++++++-- src/lib/sidenav/drawer.ts | 12 ++++++++++-- src/lib/slider/slider.ts | 13 ++++++++----- 3 files changed, 29 insertions(+), 9 deletions(-) diff --git a/src/cdk/bidi/bidi.md b/src/cdk/bidi/bidi.md index 5627d9d16bae..d9b92efb5e8c 100644 --- a/src/cdk/bidi/bidi.md +++ b/src/cdk/bidi/bidi.md @@ -6,16 +6,25 @@ text direction (RTL or LTR); #### Example ```ts @Component({ ... }) -export class MyWidget { +export class MyWidget implements OnDestroy { + + /** Whether the widget is in RTL mode or not. */ private isRtl: boolean; + /** Subscription to the Directionality change EventEmitter. */ + private _dirChangeSubscription = Subscription.EMPTY; + constructor(dir: Directionality) { this.isRtl = dir.value === 'rtl'; - dir.change.subscribe(() => { + _dirChangeSubscription = dir.change.subscribe(() => { this.flipDirection(); }); } + + ngOnDestroy() { + this._dirChangeSubscription.unsubscribe(); + } } ``` diff --git a/src/lib/sidenav/drawer.ts b/src/lib/sidenav/drawer.ts index 4a00b86a035f..51e447b709b4 100644 --- a/src/lib/sidenav/drawer.ts +++ b/src/lib/sidenav/drawer.ts @@ -31,6 +31,7 @@ import {ESCAPE} from '../core/keyboard/keycodes'; import {first, takeUntil, startWith} from '../core/rxjs/index'; import {DOCUMENT} from '@angular/platform-browser'; import {merge} from 'rxjs/observable/merge'; +import {Subscription} from 'rxjs/Subscription'; /** Throws an exception when two MdDrawer are matching the same position. */ @@ -313,7 +314,7 @@ export class MdDrawer implements AfterContentInit, OnDestroy { changeDetection: ChangeDetectionStrategy.OnPush, encapsulation: ViewEncapsulation.None, }) -export class MdDrawerContainer implements AfterContentInit { +export class MdDrawerContainer implements AfterContentInit, OnDestroy { @ContentChildren(MdDrawer) _drawers: QueryList; /** The drawer child with the `start` position. */ @@ -338,6 +339,9 @@ export class MdDrawerContainer implements AfterContentInit { private _left: MdDrawer | null; private _right: MdDrawer | null; + /** Subscription to the Directionality change EventEmitter. */ + private _dirChangeSubscription = Subscription.EMPTY; + /** Inline styles to be applied to the container. */ _styles: { marginLeft: string; marginRight: string; transform: string; }; @@ -347,7 +351,7 @@ export class MdDrawerContainer implements AfterContentInit { // If a `Dir` directive exists up the tree, listen direction changes and update the left/right // properties to point to the proper start/end. if (_dir != null) { - _dir.change.subscribe(() => this._validateDrawers()); + this._dirChangeSubscription = _dir.change.subscribe(() => this._validateDrawers()); } } @@ -361,6 +365,10 @@ export class MdDrawerContainer implements AfterContentInit { }); } + ngOnDestroy() { + this._dirChangeSubscription.unsubscribe(); + } + /** Calls `open` of both start and end drawers */ open(): void { this._drawers.forEach(drawer => drawer.open()); diff --git a/src/lib/slider/slider.ts b/src/lib/slider/slider.ts index e794e1f4575a..3bcb41ac02d0 100644 --- a/src/lib/slider/slider.ts +++ b/src/lib/slider/slider.ts @@ -38,7 +38,7 @@ import {HammerInput} from '../core'; import {FocusOrigin, FocusOriginMonitor} from '../core/style/focus-origin-monitor'; import {CanDisable, mixinDisabled} from '../core/common-behaviors/disabled'; import {CanColor, mixinColor} from '../core/common-behaviors/color'; - +import {Subscription} from 'rxjs/Subscription'; /** * Visually, a 30px separation between tick marks looks best. This is very subjective but it is @@ -385,6 +385,9 @@ export class MdSlider extends _MdSliderMixinBase /** Decimal places to round to, based on the step amount. */ private _roundLabelTo: number; + /** Subscription to the Directionality change EventEmitter. */ + private _dirChangeSubscription = Subscription.EMPTY; + /** The value of the slider when the slide start event fires. */ private _valueOnSlideStart: number | null; @@ -420,15 +423,15 @@ export class MdSlider extends _MdSliderMixinBase this._changeDetectorRef.detectChanges(); }); if (this._dir) { - this._dir.change.subscribe(() => this._changeDetectorRef.markForCheck()); + this._dirChangeSubscription = this._dir.change.subscribe(() => { + this._changeDetectorRef.markForCheck(); + }); } } ngOnDestroy() { this._focusOriginMonitor.stopMonitoring(this._elementRef.nativeElement); - if (this._dir) { - this._dir.change.unsubscribe(); - } + this._dirChangeSubscription.unsubscribe(); } _onMouseenter() {