Skip to content

Commit a7ce31e

Browse files
devversiontinayuangao
authored andcommitted
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.
1 parent bef6271 commit a7ce31e

File tree

3 files changed

+29
-9
lines changed

3 files changed

+29
-9
lines changed

src/cdk/bidi/bidi.md

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,16 +6,25 @@ text direction (RTL or LTR);
66
#### Example
77
```ts
88
@Component({ ... })
9-
export class MyWidget {
9+
export class MyWidget implements OnDestroy {
10+
11+
/** Whether the widget is in RTL mode or not. */
1012
private isRtl: boolean;
1113

14+
/** Subscription to the Directionality change EventEmitter. */
15+
private _dirChangeSubscription = Subscription.EMPTY;
16+
1217
constructor(dir: Directionality) {
1318
this.isRtl = dir.value === 'rtl';
1419

15-
dir.change.subscribe(() => {
20+
_dirChangeSubscription = dir.change.subscribe(() => {
1621
this.flipDirection();
1722
});
1823
}
24+
25+
ngOnDestroy() {
26+
this._dirChangeSubscription.unsubscribe();
27+
}
1928
}
2029
```
2130

src/lib/sidenav/drawer.ts

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ import {ESCAPE} from '../core/keyboard/keycodes';
3131
import {first, takeUntil, startWith} from '../core/rxjs/index';
3232
import {DOCUMENT} from '@angular/platform-browser';
3333
import {merge} from 'rxjs/observable/merge';
34+
import {Subscription} from 'rxjs/Subscription';
3435

3536

3637
/** Throws an exception when two MdDrawer are matching the same position. */
@@ -313,7 +314,7 @@ export class MdDrawer implements AfterContentInit, OnDestroy {
313314
changeDetection: ChangeDetectionStrategy.OnPush,
314315
encapsulation: ViewEncapsulation.None,
315316
})
316-
export class MdDrawerContainer implements AfterContentInit {
317+
export class MdDrawerContainer implements AfterContentInit, OnDestroy {
317318
@ContentChildren(MdDrawer) _drawers: QueryList<MdDrawer>;
318319

319320
/** The drawer child with the `start` position. */
@@ -338,6 +339,9 @@ export class MdDrawerContainer implements AfterContentInit {
338339
private _left: MdDrawer | null;
339340
private _right: MdDrawer | null;
340341

342+
/** Subscription to the Directionality change EventEmitter. */
343+
private _dirChangeSubscription = Subscription.EMPTY;
344+
341345
/** Inline styles to be applied to the container. */
342346
_styles: { marginLeft: string; marginRight: string; transform: string; };
343347

@@ -347,7 +351,7 @@ export class MdDrawerContainer implements AfterContentInit {
347351
// If a `Dir` directive exists up the tree, listen direction changes and update the left/right
348352
// properties to point to the proper start/end.
349353
if (_dir != null) {
350-
_dir.change.subscribe(() => this._validateDrawers());
354+
this._dirChangeSubscription = _dir.change.subscribe(() => this._validateDrawers());
351355
}
352356
}
353357

@@ -361,6 +365,10 @@ export class MdDrawerContainer implements AfterContentInit {
361365
});
362366
}
363367

368+
ngOnDestroy() {
369+
this._dirChangeSubscription.unsubscribe();
370+
}
371+
364372
/** Calls `open` of both start and end drawers */
365373
open(): void {
366374
this._drawers.forEach(drawer => drawer.open());

src/lib/slider/slider.ts

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ import {HammerInput} from '../core';
3838
import {FocusOrigin, FocusOriginMonitor} from '../core/style/focus-origin-monitor';
3939
import {CanDisable, mixinDisabled} from '../core/common-behaviors/disabled';
4040
import {CanColor, mixinColor} from '../core/common-behaviors/color';
41-
41+
import {Subscription} from 'rxjs/Subscription';
4242

4343
/**
4444
* 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
385385
/** Decimal places to round to, based on the step amount. */
386386
private _roundLabelTo: number;
387387

388+
/** Subscription to the Directionality change EventEmitter. */
389+
private _dirChangeSubscription = Subscription.EMPTY;
390+
388391
/** The value of the slider when the slide start event fires. */
389392
private _valueOnSlideStart: number | null;
390393

@@ -420,15 +423,15 @@ export class MdSlider extends _MdSliderMixinBase
420423
this._changeDetectorRef.detectChanges();
421424
});
422425
if (this._dir) {
423-
this._dir.change.subscribe(() => this._changeDetectorRef.markForCheck());
426+
this._dirChangeSubscription = this._dir.change.subscribe(() => {
427+
this._changeDetectorRef.markForCheck();
428+
});
424429
}
425430
}
426431

427432
ngOnDestroy() {
428433
this._focusOriginMonitor.stopMonitoring(this._elementRef.nativeElement);
429-
if (this._dir) {
430-
this._dir.change.unsubscribe();
431-
}
434+
this._dirChangeSubscription.unsubscribe();
432435
}
433436

434437
_onMouseenter() {

0 commit comments

Comments
 (0)