Skip to content

Commit

Permalink
fix(core): Switch emitDistinctChangesOnlyDefaultValue to true
Browse files Browse the repository at this point in the history
The previous implementation would fire changes `QueryList.changes.subscribe`
whenever the `QueryList` was recomputed. This resulted in an artificially
high number of change notifications, as it is possible that recomputing
`QueryList` results in the same list. When the `QueryList` gets recomputed
is an implementation detail, and it should not be the thing that determines
how often change event should fire.

Unfortunately, fixing the behavior outright caused too many existing
applications to fail. For this reason, Angular considers this fix a
breaking fix and has introduced a flag in `@ContentChildren` and
`@ViewChildren`, that controls the behavior.

```
export class QueryCompWithStrictChangeEmitParent {
  @ContentChildren('foo', {
    // This option is the new default with this change.
    emitDistinctChangesOnly: true,
  })
  foos!: QueryList<any>;
}
```
For backward compatibility before v12
`emitDistinctChangesOnlyDefaultValue` was set to `false. This change
changes the default to `true`.

BREAKING CHANGE: Switching default of `emitDistinctChangesOnlyDefaultValue`
which changes the default behavior and may cause some applications which
rely on the incorrect behavior to fail.
  • Loading branch information
mhevery committed Mar 8, 2021
1 parent 153e3a8 commit e976dd4
Show file tree
Hide file tree
Showing 7 changed files with 29 additions and 35 deletions.
2 changes: 1 addition & 1 deletion packages/compiler-cli/ngcc/test/rendering/renderer_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -570,7 +570,7 @@ A.ɵdir = /*@__PURE__*/ ɵngcc0.ɵɵdefineDirective({ type: A, selectors: [["",
UndecoratedBase.ɵfac = function UndecoratedBase_Factory(t) { return new (t || UndecoratedBase)(); };
// TRANSPILED
UndecoratedBase.ɵdir = /*@__PURE__*/ ɵngcc0.ɵɵdefineDirective({ type: UndecoratedBase, viewQuery: function UndecoratedBase_Query(rf, ctx) { if (rf & 1) {
ɵngcc0.ɵɵviewQuery(_c0, 3);
ɵngcc0.ɵɵviewQuery(_c0, 7);
} if (rf & 2) {
let _t;
ɵngcc0.ɵɵqueryRefresh(_t = ɵngcc0.ɵɵloadQuery()) && (ctx.test = _t.first);
Expand Down
36 changes: 18 additions & 18 deletions packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3122,10 +3122,10 @@ function allTests(os: string) {
expect(jsContents).toMatch(varRegExp('test1'));
expect(jsContents).toMatch(varRegExp('test2'));
expect(jsContents).toMatch(varRegExp('accessor'));
// match `i0.ɵɵcontentQuery(dirIndex, _c1, 1, TemplateRef)`
expect(jsContents).toMatch(contentQueryRegExp('\\w+', 1, 'TemplateRef'));
// match `i0.ɵɵviewQuery(_c2, 1, null)`
expect(jsContents).toMatch(viewQueryRegExp('\\w+', 1));
// match `i0.ɵɵcontentQuery(dirIndex, _c1, 5, TemplateRef)`
expect(jsContents).toMatch(contentQueryRegExp('\\w+', 5, 'TemplateRef'));
// match `i0.ɵɵviewQuery(_c2, 5, null)`
expect(jsContents).toMatch(viewQueryRegExp('\\w+', 5));
});

it('should generate queries for directives', () => {
Expand Down Expand Up @@ -3154,14 +3154,14 @@ function allTests(os: string) {
expect(jsContents).toMatch(varRegExp('test1'));
expect(jsContents).toMatch(varRegExp('test2'));
expect(jsContents).toMatch(varRegExp('accessor'));
// match `i0.ɵɵcontentQuery(dirIndex, _c1, 1, TemplateRef)`
expect(jsContents).toMatch(contentQueryRegExp('\\w+', 1, 'TemplateRef'));
// match `i0.ɵɵcontentQuery(dirIndex, _c1, 5, TemplateRef)`
expect(jsContents).toMatch(contentQueryRegExp('\\w+', 5, 'TemplateRef'));

// match `i0.ɵɵviewQuery(_c2, 1)`
// match `i0.ɵɵviewQuery(_c2, 5)`
// Note that while ViewQuery doesn't necessarily make sense on a directive,
// because it doesn't have a view, we still need to handle it because a component
// could extend the directive.
expect(jsContents).toMatch(viewQueryRegExp('\\w+', 1));
expect(jsContents).toMatch(viewQueryRegExp('\\w+', 5));
});

it('should handle queries that use forwardRef', () => {
Expand All @@ -3183,13 +3183,13 @@ function allTests(os: string) {

env.driveMain();
const jsContents = env.getContents('test.js');
// match `i0.ɵɵcontentQuery(dirIndex, TemplateRef, 1, null)`
expect(jsContents).toMatch(contentQueryRegExp('TemplateRef', 1));
// match `i0.ɵɵcontentQuery(dirIndex, ViewContainerRef, 1, null)`
expect(jsContents).toMatch(contentQueryRegExp('ViewContainerRef', 1));
// match `i0.ɵɵcontentQuery(dirIndex, _c0, 1, null)`
// match `i0.ɵɵcontentQuery(dirIndex, TemplateRef, 5, null)`
expect(jsContents).toMatch(contentQueryRegExp('TemplateRef', 5));
// match `i0.ɵɵcontentQuery(dirIndex, ViewContainerRef, 5, null)`
expect(jsContents).toMatch(contentQueryRegExp('ViewContainerRef', 5));
// match `i0.ɵɵcontentQuery(dirIndex, _c0, 5, null)`
expect(jsContents).toContain('_c0 = ["parens"];');
expect(jsContents).toMatch(contentQueryRegExp('_c0', 1));
expect(jsContents).toMatch(contentQueryRegExp('_c0', 5));
});

it('should handle queries that use an InjectionToken', () => {
Expand All @@ -3210,10 +3210,10 @@ function allTests(os: string) {

env.driveMain();
const jsContents = env.getContents('test.js');
// match `i0.ɵɵviewQuery(TOKEN, 1, null)`
expect(jsContents).toMatch(viewQueryRegExp('TOKEN', 1));
// match `i0.ɵɵcontentQuery(dirIndex, TOKEN, 1, null)`
expect(jsContents).toMatch(contentQueryRegExp('TOKEN', 1));
// match `i0.ɵɵviewQuery(TOKEN, 5, null)`
expect(jsContents).toMatch(viewQueryRegExp('TOKEN', 5));
// match `i0.ɵɵcontentQuery(dirIndex, TOKEN, 5, null)`
expect(jsContents).toMatch(contentQueryRegExp('TOKEN', 5));
});

it('should compile expressions that write keys', () => {
Expand Down
5 changes: 2 additions & 3 deletions packages/compiler/src/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,8 @@ export const createAttribute =
makeMetadataFactory<Attribute>('Attribute', (attributeName: string) => ({attributeName}));

// Stores the default value of `emitDistinctChangesOnly` when the `emitDistinctChangesOnly` is not
// explicitly set. This value will be changed to `true` in v12.
// TODO(misko): switch the default in v12 to `true`. See: packages/core/src/metadata/di.ts
export const emitDistinctChangesOnlyDefaultValue = false;
// explicitly set.
export const emitDistinctChangesOnlyDefaultValue = true;


export interface Query {
Expand Down
3 changes: 0 additions & 3 deletions packages/compiler/src/render3/partial/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -237,9 +237,6 @@ export interface R3DeclareQueryMetadata {
/**
* True to only fire changes if there are underlying changes to the query.
*/
// TODO(misko): This will become `true` be default in v12. `QueryList.changes` would fire even if
// no changes to the query list were detected. This is not ideal, as changes should only fire if
// the `QueryList` actually materially changed.
emitDistinctChangesOnly?: boolean;

/**
Expand Down
3 changes: 1 addition & 2 deletions packages/core/src/linker/query_list.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,7 @@ export class QueryList<T> implements Iterable<T> {
/**
* @param emitDistinctChangesOnly Whether `QueryList.changes` should fire only when actual change
* has occurred. Or if it should fire when query is recomputed. (recomputing could resolve in
* the same result) This is set to `false` for backwards compatibility but will be changed to
* true in v12.
* the same result)
*/
constructor(private _emitDistinctChangesOnly: boolean = false) {
// This function should be declared on the prototype, but doing so there will cause the class
Expand Down
13 changes: 6 additions & 7 deletions packages/core/src/metadata/di.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,8 @@ export interface Query {
}

// Stores the default value of `emitDistinctChangesOnly` when the `emitDistinctChangesOnly` is not
// explicitly set. This value will be changed to `true` in v12.
// TODO(misko): switch the default in v12 to `true`. See: packages/compiler/src/core.ts
export const emitDistinctChangesOnlyDefaultValue = false;
// explicitly set.
export const emitDistinctChangesOnlyDefaultValue = true;


/**
Expand Down Expand Up @@ -148,8 +147,8 @@ export interface ContentChildrenDecorator {
* * **selector** - The directive type or the name used for querying.
* * **descendants** - True to include all descendants, otherwise include only direct children.
* * **emitDistinctChangesOnly** - The ` QueryList#changes` observable will emit new values only
* if the QueryList result has changed. The default value will change from `false` to `true` in
* v12. When `false` the `changes` observable might emit even if the QueryList has not changed.
* if the QueryList result has changed. When `false` the `changes` observable might emit even
* if the QueryList has not changed.
* * **read** - Used to read a different token from the queried elements.
*
* @usageNotes
Expand Down Expand Up @@ -287,8 +286,8 @@ export interface ViewChildrenDecorator {
* * **selector** - The directive type or the name used for querying.
* * **read** - Used to read a different token from the queried elements.
* * **emitDistinctChangesOnly** - The ` QueryList#changes` observable will emit new values only
* if the QueryList result has changed. The default value will change from `false` to `true` in
* v12. When `false` the `changes` observable might emit even if the QueryList has not changed.
* if the QueryList result has changed. When `false` the `changes` observable might emit even
* if the QueryList has not changed.
*
* @usageNotes
*
Expand Down
2 changes: 1 addition & 1 deletion packages/core/test/acceptance/query_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1298,7 +1298,7 @@ describe('query logic', () => {
vc.move(viewRef, 0);
fixture.detectChanges();
expect(queryList.length).toBe(1);
expect(fixture.componentInstance.queryListNotificationCounter).toBe(2);
expect(fixture.componentInstance.queryListNotificationCounter).toBe(1);
});

it('should support a mix of content queries from the declaration and embedded view', () => {
Expand Down

0 comments on commit e976dd4

Please sign in to comment.