fix(select): prevent change detection of destroyed option (#1329)

* fix(select): prevent detect changes call on destroyed option

* test(select): check option ignores selection change if destroyed

* fix(select): prevent detect changes call on destroyed select

* test(select): start cd after components marked for check asynchronously

* fix(select): ignore write value if select component destroyed

src/framework/theme/components/select/option.component.ts

@@ -100,23 +100,25 @@ export class NbOptionComponent<T> implements OnDestroy {
   }
 
   select() {
-    this.selected = true;
-    this.cd.markForCheck();
-    this.cd.detectChanges();
+    this.setSelection(true);
   }
 
   deselect() {
+    this.setSelection(false);
+  }
+
+  private setSelection(isSelected: boolean): void {
     /**
      * In case of changing options in runtime the reference to the selected option will be kept in select component.
      * This may lead to exceptions with detecting changes in destroyed component.
+     *
+     * Also Angular can call writeValue on destroyed view (select implements ControlValueAccessor).
+     * angular/angular#27803
      * */
-    if (!this.alive) {
-      return;
+    if (this.alive) {
+      this.selected = isSelected;
+      this.cd.markForCheck();
     }
-
-    this.selected = false;
-    this.cd.markForCheck();
-    this.cd.detectChanges();
   }
 }
 

src/framework/theme/components/select/select.component.ts

@@ -25,8 +25,8 @@ import {
   ViewChild,
 } from '@angular/core';
 import { ControlValueAccessor, NG_VALUE_ACCESSOR } from '@angular/forms';
-import { take, takeWhile } from 'rxjs/operators';
 import { merge, Observable, Subject } from 'rxjs';
+import { takeWhile } from 'rxjs/operators';
 
 import {
   NbAdjustableConnectedPositionStrategy,
@@ -373,11 +373,11 @@ export class NbSelectComponent<T> implements OnInit, AfterViewInit, AfterContent
 
   setDisabledState(isDisabled: boolean): void {
     this.disabled = isDisabled;
-    this.cd.detectChanges();
+    this.cd.markForCheck();
   }
 
   writeValue(value: T | T[]): void {
-    if (!value) {
+    if (!value || !this.alive) {
       return;
     }
 
@@ -398,7 +398,7 @@ export class NbSelectComponent<T> implements OnInit, AfterViewInit, AfterContent
       this.reset();
     }
 
-    this.cd.detectChanges();
+    this.cd.markForCheck();
   }
 
   /**
@@ -495,11 +495,10 @@ export class NbSelectComponent<T> implements OnInit, AfterViewInit, AfterContent
   protected subscribeOnPositionChange() {
     this.positionStrategy.positionChange
       .pipe(takeWhile(() => this.alive))
-      .subscribe((position: NbPosition) => this.overlayPosition = position);
-
-    this.positionStrategy.positionChange
-      .pipe(take(1))
-      .subscribe(() => this.cd.detectChanges());
+      .subscribe((position: NbPosition) => {
+        this.overlayPosition = position;
+        this.cd.detectChanges();
+      });
   }
 
   protected subscribeOnSelectionChange() {
@@ -556,7 +555,6 @@ export class NbSelectComponent<T> implements OnInit, AfterViewInit, AfterContent
     }
 
     this.cd.markForCheck();
-    this.cd.detectChanges();
   }
 
   protected cleanSelection() {

src/framework/theme/components/select/select.spec.ts

@@ -6,15 +6,17 @@
 
 import { Component, EventEmitter, Input, Output, ViewChild } from '@angular/core';
 import { ComponentFixture, fakeAsync, flush, TestBed } from '@angular/core/testing';
+import { FormControl, FormsModule, ReactiveFormsModule } from '@angular/forms';
+import { RouterTestingModule } from '@angular/router/testing';
+import { from, zip } from 'rxjs';
 
 import { NbSelectModule } from './select.module';
 import { NbThemeModule } from '../../theme.module';
 import { NbOverlayContainerAdapter } from '../cdk/adapter/overlay-container-adapter';
 import { NB_DOCUMENT } from '../../theme.options';
 import { NbSelectComponent } from './select.component';
 import { NbLayoutModule } from '../layout/layout.module';
-import { RouterTestingModule } from '@angular/router/testing';
-import { from, zip } from 'rxjs';
+import { NbOptionComponent } from './option.component';
 
 
 const TEST_GROUPS = [
@@ -93,6 +95,27 @@ export class NbSelectWithInitiallySelectedOptionComponent {
   @ViewChild(NbSelectComponent) select: NbSelectComponent<number>;
 }
 
+@Component({
+  template: `
+    <nb-layout>
+      <nb-layout-column>
+
+        <nb-select *ngIf="showSelect" [formControl]="formControl">
+          <nb-option *ngFor="let option of options" [value]="option">{{ option }}</nb-option>
+        </nb-select>
+
+      </nb-layout-column>
+    </nb-layout>
+  `,
+})
+export class NbReactiveFormSelectComponent {
+  options: number[] = [ 1 ];
+  showSelect: boolean = true;
+  formControl: FormControl = new FormControl();
+
+  @ViewChild(NbOptionComponent) optionComponent: NbOptionComponent<number>;
+}
+
 describe('Component: NbSelectComponent', () => {
   let fixture: ComponentFixture<NbSelectTestComponent>;
   let overlayContainerService: NbOverlayContainerAdapter;
@@ -110,11 +133,17 @@ describe('Component: NbSelectComponent', () => {
     TestBed.configureTestingModule({
       imports: [
         RouterTestingModule.withRoutes([]),
+        FormsModule,
+        ReactiveFormsModule,
         NbThemeModule.forRoot(),
         NbLayoutModule,
         NbSelectModule,
       ],
-      declarations: [ NbSelectTestComponent, NbSelectWithInitiallySelectedOptionComponent ],
+      declarations: [
+        NbSelectTestComponent,
+        NbSelectWithInitiallySelectedOptionComponent,
+        NbReactiveFormSelectComponent,
+      ],
     });
 
     fixture = TestBed.createComponent(NbSelectTestComponent);
@@ -244,10 +273,35 @@ describe('Component: NbSelectComponent', () => {
     const selectFixture = TestBed.createComponent(NbSelectWithInitiallySelectedOptionComponent);
     selectFixture.detectChanges();
     flush();
+    selectFixture.detectChanges();
 
     const selectedOption = selectFixture.componentInstance.select.options.find(o => o.selected);
     expect(selectedOption.value).toEqual(selectFixture.componentInstance.selected);
     const selectButton = selectFixture.nativeElement.querySelector('nb-select button') as HTMLElement;
     expect(selectButton.textContent).toEqual(selectedOption.value.toString());
   }));
+
+  describe('NbOptionComponent', () => {
+    it('should ignore selection change if destroyed', () => {
+      const selectFixture = TestBed.createComponent(NbReactiveFormSelectComponent);
+      const testSelectComponent = selectFixture.componentInstance;
+      selectFixture.detectChanges();
+
+      const option = testSelectComponent.optionComponent;
+      const markForCheckSpy = spyOn((option as any).cd, 'markForCheck').and.callThrough();
+
+      testSelectComponent.formControl.setValue(1);
+      selectFixture.detectChanges();
+
+      expect(option.selected).toEqual(true);
+      expect(markForCheckSpy).toHaveBeenCalledTimes(1);
+
+      testSelectComponent.showSelect = false;
+      selectFixture.detectChanges();
+
+      expect(() => testSelectComponent.formControl.setValue(2)).not.toThrow();
+      expect(option.selected).toEqual(true);
+      expect(markForCheckSpy).toHaveBeenCalledTimes(1);
+    });
+  });
 });