Skip to content

Commit f82259b

Browse files
andrewseguinmmalerba
authored andcommitted
fix(table): use default change detection strategy on table (#15440)
This resolves an issue with Ivy where change detection was not reflecting binding changes made to the row and cell templates in the component declaring the table. In View Engine, change detection for a dynamically created view runs in two cases: 1) When the view in which it was inserted is checked 2) When the view in which it was declared is checked As a result, if a dynamic view is inserted into an OnPush component, that view is not actually checked only "on push". It is also checked as often as its declaration view is checked (even if the insertion view is explicitly detached from change detection). For this reason, marking `MatTable` as "OnPush" doesn't have much of an effect. It is built almost entirely of views that are declared elsewhere, so its change detection frequency is dependent on those declaration views. It also doesn't have any bindings inside its own view that would be protected by "OnPush", so marking it this way is effectively a noop and can be removed without performance regressions. In Ivy, this change detection loophole has been closed, so declaration views can no longer de-optimize OnPush components. This means bindings inherited from declaration views aren't updated by default if `MatTable` is OnPush (the embedded view would need to be marked dirty explicitly). To avoid breaking changes when we transition to Ivy, it makes more sense to perpetuate the current behavior by removing the "OnPush" marker. As delineated earlier, this has no impact on View Engine. For Ivy, it would ensure bindings inherited from the declaration view are still checked by default.
1 parent decddb5 commit f82259b

File tree

5 files changed

+138
-106
lines changed

5 files changed

+138
-106
lines changed

src/cdk/table/row.ts

Lines changed: 25 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import {
1818
SimpleChanges,
1919
TemplateRef,
2020
ViewContainerRef,
21-
ViewEncapsulation,
21+
ViewEncapsulation
2222
} from '@angular/core';
2323
import {CanStick, CanStickCtor, mixinHasStickyInput} from './can-stick';
2424
import {CdkCellDef, CdkColumnDef} from './cell';
@@ -40,8 +40,9 @@ export abstract class BaseRowDef implements OnChanges {
4040
/** Differ used to check if any changes were made to the columns. */
4141
protected _columnsDiffer: IterableDiffer<any>;
4242

43-
constructor(/** @docs-private */ public template: TemplateRef<any>,
44-
protected _differs: IterableDiffers) { }
43+
constructor(
44+
/** @docs-private */ public template: TemplateRef<any>, protected _differs: IterableDiffers) {
45+
}
4546

4647
ngOnChanges(changes: SimpleChanges): void {
4748
// Create a new columns differ if one does not yet exist. Initialize it based on initial value
@@ -57,15 +58,16 @@ export abstract class BaseRowDef implements OnChanges {
5758
* Returns the difference between the current columns and the columns from the last diff, or null
5859
* if there is no difference.
5960
*/
60-
getColumnsDiff(): IterableChanges<any> | null {
61+
getColumnsDiff(): IterableChanges<any>|null {
6162
return this._columnsDiffer.diff(this.columns);
6263
}
6364

6465
/** Gets this row def's relevant cell template from the provided column def. */
6566
extractCellTemplate(column: CdkColumnDef): TemplateRef<any> {
6667
if (this instanceof CdkHeaderRowDef) {
6768
return column.headerCell.template;
68-
} if (this instanceof CdkFooterRowDef) {
69+
}
70+
if (this instanceof CdkFooterRowDef) {
6971
return column.footerCell.template;
7072
} else {
7173
return column.cell.template;
@@ -76,7 +78,7 @@ export abstract class BaseRowDef implements OnChanges {
7678
// Boilerplate for applying mixins to CdkHeaderRowDef.
7779
/** @docs-private */
7880
export class CdkHeaderRowDefBase extends BaseRowDef {}
79-
export const _CdkHeaderRowDefBase: CanStickCtor & typeof CdkHeaderRowDefBase =
81+
export const _CdkHeaderRowDefBase: CanStickCtor&typeof CdkHeaderRowDefBase =
8082
mixinHasStickyInput(CdkHeaderRowDefBase);
8183

8284
/**
@@ -102,7 +104,7 @@ export class CdkHeaderRowDef extends _CdkHeaderRowDefBase implements CanStick, O
102104
// Boilerplate for applying mixins to CdkFooterRowDef.
103105
/** @docs-private */
104106
export class CdkFooterRowDefBase extends BaseRowDef {}
105-
export const _CdkFooterRowDefBase: CanStickCtor & typeof CdkFooterRowDefBase =
107+
export const _CdkFooterRowDefBase: CanStickCtor&typeof CdkFooterRowDefBase =
106108
mixinHasStickyInput(CdkFooterRowDefBase);
107109

108110
/**
@@ -224,7 +226,7 @@ export class CdkCellOutlet implements OnDestroy {
224226
* a handle to provide that component's cells and context. After init, the CdkCellOutlet will
225227
* construct the cells with the provided context.
226228
*/
227-
static mostRecentCellOutlet: CdkCellOutlet | null = null;
229+
static mostRecentCellOutlet: CdkCellOutlet|null = null;
228230

229231
constructor(public _viewContainer: ViewContainerRef) {
230232
CdkCellOutlet.mostRecentCellOutlet = this;
@@ -248,10 +250,13 @@ export class CdkCellOutlet implements OnDestroy {
248250
'class': 'cdk-header-row',
249251
'role': 'row',
250252
},
251-
changeDetection: ChangeDetectionStrategy.OnPush,
253+
// See note on CdkTable for explanation on why this uses the default change detection strategy.
254+
// tslint:disable-next-line:validate-decorators
255+
changeDetection: ChangeDetectionStrategy.Default,
252256
encapsulation: ViewEncapsulation.None,
253257
})
254-
export class CdkHeaderRow { }
258+
export class CdkHeaderRow {
259+
}
255260

256261

257262
/** Footer template container that contains the cell outlet. Adds the right class and role. */
@@ -263,10 +268,13 @@ export class CdkHeaderRow { }
263268
'class': 'cdk-footer-row',
264269
'role': 'row',
265270
},
266-
changeDetection: ChangeDetectionStrategy.OnPush,
271+
// See note on CdkTable for explanation on why this uses the default change detection strategy.
272+
// tslint:disable-next-line:validate-decorators
273+
changeDetection: ChangeDetectionStrategy.Default,
267274
encapsulation: ViewEncapsulation.None,
268275
})
269-
export class CdkFooterRow { }
276+
export class CdkFooterRow {
277+
}
270278

271279
/** Data row template container that contains the cell outlet. Adds the right class and role. */
272280
@Component({
@@ -277,7 +285,10 @@ export class CdkFooterRow { }
277285
'class': 'cdk-row',
278286
'role': 'row',
279287
},
280-
changeDetection: ChangeDetectionStrategy.OnPush,
288+
// See note on CdkTable for explanation on why this uses the default change detection strategy.
289+
// tslint:disable-next-line:validate-decorators
290+
changeDetection: ChangeDetectionStrategy.Default,
281291
encapsulation: ViewEncapsulation.None,
282292
})
283-
export class CdkRow { }
293+
export class CdkRow {
294+
}

0 commit comments

Comments
 (0)