fix(table): eliminate need for second change detection (#5775)

* fix(table): eliminate need for second change detection

* change var

* move row clear

* add comment regarding change det

src/cdk/table/data-source.ts

@@ -14,7 +14,9 @@ export interface CollectionViewer {
 
 export abstract class DataSource<T> {
   /**
-   * Connects a collection viewer (such as a data-table) to this data source.
+   * Connects a collection viewer (such as a data-table) to this data source. Note that
+   * the stream provided will be accessed during change detection and should not directly change
+   * values that are bound in template views.
    * @param collectionViewer The component that exposes a view over the data provided by this
    *     data source.
    * @returns Observable that emits a new value when the data changes.

src/cdk/table/row.ts

@@ -51,8 +51,10 @@ export abstract class BaseRowDef {
   ngOnChanges(changes: SimpleChanges): void {
     // Create a new columns differ if one does not yet exist. Initialize it based on initial value
     // of the columns property.
-    if (!this._columnsDiffer && changes['columns'].currentValue) {
-      this._columnsDiffer = this._differs.find(changes['columns'].currentValue).create();
+    const columns = changes['columns'].currentValue;
+    if (!this._columnsDiffer && columns) {
+      this._columnsDiffer = this._differs.find(columns).create();
+      this._columnsDiffer.diff(columns);
     }
   }
 

src/cdk/table/table.spec.ts

@@ -37,8 +37,7 @@ describe('CdkTable', () => {
     table = component.table;
     tableElement = fixture.nativeElement.querySelector('cdk-table');
 
-    fixture.detectChanges();  // Let the component and table create embedded views
-    fixture.detectChanges();  // Let the cells render
+    fixture.detectChanges();
   });
 
   describe('should initialize', () => {
@@ -120,8 +119,6 @@ describe('CdkTable', () => {
     });
   });
 
-  // TODO(andrewseguin): Add test for dynamic classes on header/rows
-
   it('should use differ to add/remove/move rows', () => {
     // Each row receives an attribute 'initialIndex' the element's original place
     getRows(tableElement).forEach((row: Element, index: number) => {
@@ -182,9 +179,7 @@ describe('CdkTable', () => {
       dataSource = trackByComponent.dataSource as FakeDataSource;
       table = trackByComponent.table;
       tableElement = trackByFixture.nativeElement.querySelector('cdk-table');
-
-      trackByFixture.detectChanges();  // Let the component and table create embedded views
-      trackByFixture.detectChanges();  // Let the cells render
+      trackByFixture.detectChanges();
 
       // Each row receives an attribute 'initialIndex' the element's original place
       getRows(tableElement).forEach((row: Element, index: number) => {
@@ -307,12 +302,10 @@ describe('CdkTable', () => {
   });
 
   it('should match the right table content with dynamic data source', () => {
-    fixture = TestBed.createComponent(DynamicDataSourceCdkTableApp);
-    component = fixture.componentInstance;
-    tableElement = fixture.nativeElement.querySelector('cdk-table');
-
-    fixture.detectChanges();  // Let the table render the rows
-    fixture.detectChanges();  // Let the rows render their cells
+    const dynamicDataSourceFixture = TestBed.createComponent(DynamicDataSourceCdkTableApp);
+    component = dynamicDataSourceFixture.componentInstance;
+    tableElement = dynamicDataSourceFixture.nativeElement.querySelector('cdk-table');
+    dynamicDataSourceFixture.detectChanges();
 
     // Expect that the component has no data source and the table element reflects empty data.
     expect(component.dataSource).toBe(undefined);
@@ -323,10 +316,10 @@ describe('CdkTable', () => {
     // Add a data source that has initialized data. Expect that the table shows this data.
     const dynamicDataSource = new FakeDataSource();
     component.dataSource = dynamicDataSource;
-    fixture.detectChanges();
+    dynamicDataSourceFixture.detectChanges();
     expect(dynamicDataSource.isConnected).toBe(true);
 
-    let data = component.dataSource.data;
+    const data = component.dataSource.data;
     expectTableToMatchContent(tableElement, [
       ['Column A'],
       [data[0].a],
@@ -336,24 +329,36 @@ describe('CdkTable', () => {
 
     // Remove the data source and check to make sure the table is empty again.
     component.dataSource = null;
-    fixture.detectChanges();
+    dynamicDataSourceFixture.detectChanges();
 
     // Expect that the old data source has been disconnected.
     expect(dynamicDataSource.isConnected).toBe(false);
     expectTableToMatchContent(tableElement, [
       ['Column A']
     ]);
+
+    // Reconnect a data source and check that the table is populated
+    const newDynamicDataSource = new FakeDataSource();
+    component.dataSource = newDynamicDataSource;
+    dynamicDataSourceFixture.detectChanges();
+    expect(newDynamicDataSource.isConnected).toBe(true);
+
+    const newData = component.dataSource.data;
+    expectTableToMatchContent(tableElement, [
+      ['Column A'],
+      [newData[0].a],
+      [newData[1].a],
+      [newData[2].a],
+    ]);
   });
 
   it('should be able to apply classes to rows based on their context', () => {
     const contextFixture = TestBed.createComponent(RowContextCdkTableApp);
     const contextComponent = contextFixture.componentInstance;
     tableElement = contextFixture.nativeElement.querySelector('cdk-table');
+    contextFixture.detectChanges();
 
-    contextFixture.detectChanges();  // Let the table initialize its view
-    contextFixture.detectChanges();  // Let the table render the rows and cells
-
-    const rowElements = contextFixture.nativeElement.querySelectorAll('cdk-row');
+    let rowElements = contextFixture.nativeElement.querySelectorAll('cdk-row');
 
     // Rows should not have any context classes
     for (let i = 0; i < rowElements.length; i++) {
@@ -387,9 +392,7 @@ describe('CdkTable', () => {
     const contextFixture = TestBed.createComponent(RowContextCdkTableApp);
     const contextComponent = contextFixture.componentInstance;
     tableElement = contextFixture.nativeElement.querySelector('cdk-table');
-
-    contextFixture.detectChanges();  // Let the table initialize its view
-    contextFixture.detectChanges();  // Let the table render the rows and cells
+    contextFixture.detectChanges();
 
     const rowElements = contextFixture.nativeElement.querySelectorAll('cdk-row');
 
@@ -681,7 +684,7 @@ function expectTableToMatchContent(tableElement: Element, expectedTableContent:
   // Check data row cells
   getRows(tableElement).forEach((row, rowIndex) => {
     getCells(row).forEach((cell, cellIndex) => {
-      const expected = expectedHeaderContent ?
+      const expected = expectedTableContent.length ?
           expectedTableContent[rowIndex][cellIndex] :
           null;
       checkCellContent(cell, expected);

src/cdk/table/table.ts

@@ -90,14 +90,11 @@ export class CdkTable<T> implements CollectionViewer {
   /** Subject that emits when the component has been destroyed. */
   private _onDestroy = new Subject<void>();
 
-  /** Flag set to true after the component has been initialized. */
-  private _isViewInitialized = false;
-
   /** Latest data provided by the data source through the connect interface. */
   private _data: NgIterable<T> = [];
 
   /** Subscription that listens for the data provided by the data source. */
-  private _renderChangeSubscription: Subscription;
+  private _renderChangeSubscription: Subscription | null;
 
   /**
    * Map of all the user's defined columns identified by name.
@@ -188,6 +185,7 @@ export class CdkTable<T> implements CollectionViewer {
   ngOnInit() {
     // TODO(andrewseguin): Setup a listener for scroll events
     //   and emit the calculated view to this.viewChange
+    this._dataDiffer = this._differs.find([]).create(this._trackByFn);
   }
 
   ngAfterContentInit() {
@@ -212,20 +210,13 @@ export class CdkTable<T> implements CollectionViewer {
       this._headerRowPlaceholder.viewContainer.clear();
       this._renderHeaderRow();
     });
-  }
 
-  ngAfterViewInit() {
-    // Find and construct an iterable differ that can be used to find the diff in an array.
-    this._dataDiffer = this._differs.find([]).create(this._trackByFn);
-    this._isViewInitialized = true;
+    this._renderHeaderRow();
   }
 
-  ngDoCheck() {
-    if (this._isViewInitialized && this.dataSource && !this._renderChangeSubscription) {
-      this._renderHeaderRow();
-      if (this.dataSource && !this._renderChangeSubscription) {
-        this._observeRenderChanges();
-      }
+  ngAfterContentChecked() {
+    if (this.dataSource && !this._renderChangeSubscription) {
+      this._observeRenderChanges();
     }
   }
 
@@ -237,22 +228,22 @@ export class CdkTable<T> implements CollectionViewer {
   private _switchDataSource(dataSource: DataSource<T>) {
     this._data = [];
 
-    if (this._dataSource) {
+    if (this.dataSource) {
       this.dataSource.disconnect(this);
     }
-    this._dataSource = dataSource;
 
-    if (this._isViewInitialized) {
-      if (this._renderChangeSubscription) {
-        this._renderChangeSubscription.unsubscribe();
-      }
+    // Stop listening for data from the previous data source.
+    if (this._renderChangeSubscription) {
+      this._renderChangeSubscription.unsubscribe();
+      this._renderChangeSubscription = null;
+    }
 
-      if (this._dataSource) {
-        this._observeRenderChanges();
-      } else {
-        this._rowPlaceholder.viewContainer.clear();
-      }
+    // Remove the table's rows if there is now no data source
+    if (!dataSource) {
+      this._rowPlaceholder.viewContainer.clear();
     }
+
+    this._dataSource = dataSource;
   }
 
   /** Set up a subscription for the data provided by the data source. */