fix(core): `QueryList` does not fire changes if the underlying list did not change. #40091

mhevery · 2020-12-11T18:45:47Z

Previous implementation would fire changes QueryList.changes.subscribe
even if underlying list did not change. Such situation can arise if a
LView is inserted or removed, causing QueryList to recompute, but
the recompute results in the same exact QueryList result. In such
a case we don't want to fire a change event (as there is no change.)

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Other information

mhevery · 2020-12-11T18:49:56Z

presubmit

mhevery · 2020-12-11T20:44:47Z

presubmit ( deflake )

IgorMinar

Overall this looks good, thanks for debugging this.

Could you instead consider using the native Array#flat as this would simplify the code and in fact remove bulk of it.

We'll then also need to add

import 'core-js/modules/es.array.flat';
import 'core-js/modules/es.array.flat-map';

to

https://github.com/angular/angular-cli/blob/0d10de5cbba9f4eaf91e0fbc279a1f3e0ad94fa3/packages/angular_devkit/build_angular/src/webpack/es5-polyfills.js#L36-L41

just so that we don't break on IE11 (the flat-map addition is not required by this change, but we might as well add it for future use).

If we target this fix for target: minor then we can roll it out now since CLI would add the polyfill to ie11 builds in v11.1 as well.

packages/core/src/linker/query_list.ts

packages/core/src/util/array_utils.ts

packages/core/test/util/array_utils_spec.ts

AndrewKushnir

LGTM (from code perspective), thanks for the fix @mhevery 👍

I was also thinking of using Array.flat function as @IgorMinar proposed, but that'd result in additional array allocation and extra pass to compare current/new arrays (which is avoided in the current implementation). Given that QueryList.reset is called for each CD run (executed from refreshView fn), it looks like that might sensitive from perf perspective...

packages/core/src/util/array_utils.ts

packages/core/test/acceptance/query_spec.ts

packages/core/src/util/array_utils.ts

mhevery · 2020-12-14T21:36:02Z

@IgorMinar Array#flat seems like a lot of work, so I am going to do it in a separate PR since it includes changes to polyfills and I don't think we should be changing polyfills in patch release.

AndrewKushnir · 2020-12-15T00:33:30Z

packages/core/src/linker/query_list.ts

    (this.changes as EventEmitter<any>).complete();
    (this.changes as EventEmitter<any>).unsubscribe();


We should probably do that for both _changesDeprecated and _changesStrict event emitters (in case both emitters were used in an app).

AndrewKushnir · 2020-12-15T00:37:52Z

packages/core/src/linker/query_list.ts

+      (this as {length: number}).length = this._results.length;
+      (this as {last: T}).last = this._results[this.length - 1];
+      (this as {first: T}).first = this._results[0];


This is outside of the context of this PR, but it feels like these fields should be just getters on the QueryList class (so setting their values would not be required) and that should also help with typings above.

packages/core/test/acceptance/query_spec.ts

AndrewKushnir · 2020-12-15T06:17:59Z

FYI, started global presubmit.

JoostK

I'd personally mark this for minor and not patch, since it introduces new API.

JoostK · 2020-12-15T20:29:30Z

packages/core/src/linker/query_list.ts

@@ -135,28 +169,36 @@ export class QueryList<T> implements Iterable<T> {
   * occurs.
   *
   * @param resultsTree The query results to store
+   * @returns `true` only if the new `resultTree` resulted in a different query set.
+   * @internal


Although this is marked @internal, it still shows up in the public api. As this is targeting patch I don't think we can/should hide this method in a patch release.

This is an internal method which should never be called by anyone. I can take it out if you feel strongly about it.

So I'm fine with marking it internal, it's just that doing so could break projects who call this method manually. So we may need to go through a deprecation period.

packages/core/src/linker/query_list.ts

mhevery · 2020-12-15T23:42:57Z

presubmit

AndrewKushnir · 2020-12-16T05:34:12Z

Global presubmit #2 (using Ivy).

mhevery · 2021-01-13T00:28:07Z

presubmit (deflake)

mary-poppins · 2021-01-13T00:40:08Z

You can preview af6d6cd at https://pr40091-af6d6cd.ngbuilds.io/.

IgorMinar

LGTM

Reviewed-for: size-tracking, public-api

mary-poppins · 2021-01-13T18:01:05Z

You can preview 29c4f20 at https://pr40091-29c4f20.ngbuilds.io/.

mary-poppins · 2021-01-13T21:49:47Z

You can preview 469090e at https://pr40091-469090e.ngbuilds.io/.

packages/compiler-cli/linker/src/file_linker/partial_linkers/partial_directive_linker_1.ts

mary-poppins · 2021-01-14T20:14:07Z

You can preview 4d81a44 at https://pr40091-4d81a44.ngbuilds.io/.

@ContentChildren

… did not change. Previous implementation would fire changes `QueryList.changes.subscribe` whenever the `QueryList` was recomputed. This resulted in 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 which determines how often change event should fire. This change introduces a new `emitDistinctChangesOnly` option for `ContentChildren` and `ViewChildren`. ``` export class QueryCompWithStrictChangeEmitParent { @ContentChildren('foo', { // This option will become the default in the future emitDistinctChangesOnly: true, }) foos!: QueryList<any>; } ```

Because the query now has `flags` which specify the mode, the static query instruction can now be remove. It is simply normal query with `static` flag.

mary-poppins · 2021-01-14T21:38:57Z

You can preview 29cd0de at https://pr40091-29cd0de.ngbuilds.io/.

@ContentChildren

… did not change. (#40091) Previous implementation would fire changes `QueryList.changes.subscribe` whenever the `QueryList` was recomputed. This resulted in 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 which determines how often change event should fire. This change introduces a new `emitDistinctChangesOnly` option for `ContentChildren` and `ViewChildren`. ``` export class QueryCompWithStrictChangeEmitParent { @ContentChildren('foo', { // This option will become the default in the future emitDistinctChangesOnly: true, }) foos!: QueryList<any>; } ``` PR Close #40091

…#40091) Because the query now has `flags` which specify the mode, the static query instruction can now be remove. It is simply normal query with `static` flag. PR Close #40091

AndrewKushnir · 2021-01-14T21:56:20Z

@mhevery this PR was merged into RC (11.1.x) and master branches, but there was a conflict of merging it to the patch branch (11.0.x). Could you please create a patch-only version of this change if needed? Thank you.

JoostK · 2021-01-14T22:08:46Z

@mhevery this PR was merged into RC (11.1.x) and master branches, but there was a conflict of merging it to the patch branch (11.0.x). Could you please create a patch-only version of this change if needed? Thank you.

I believe RC should be sufficient; there's no real need for this to land in 11.0.x with 11.1 just around the corner (there might not even be another 11.0.x release?) and porting this is non-trivial as the linker code doesn't exist in 11.0.x.

AndrewKushnir · 2021-01-14T22:18:57Z

Thanks @JoostK. Right, we also discussed that with @mhevery and agreed to keep this change in RC and master only (no changes to the patch branch).

angular-automatic-lock-bot · 2021-02-14T16:34:17Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

google-cla bot added the cla: yes label Dec 11, 2020

pullapprove bot added the area: core Issues related to the framework runtime label Dec 11, 2020

ngbot bot added this to the Backlog milestone Dec 11, 2020

mhevery force-pushed the query_stable_notification branch 2 times, most recently from 77480e0 to f02babb Compare December 11, 2020 20:41

mhevery force-pushed the query_stable_notification branch 2 times, most recently from 2cd900f to 266fa09 Compare December 11, 2020 23:35

pullapprove bot requested review from AndrewKushnir, atscott, IgorMinar and jelbourn December 11, 2020 23:35

pullapprove bot added the comp: docs/api label Dec 11, 2020

IgorMinar suggested changes Dec 12, 2020

View reviewed changes

pullapprove bot requested a review from IgorMinar December 12, 2020 00:14

mhevery added the target: patch This PR is targeted for the next patch release label Dec 12, 2020

AndrewKushnir approved these changes Dec 12, 2020

View reviewed changes

packages/core/src/util/array_utils.ts Outdated Show resolved Hide resolved

packages/core/test/acceptance/query_spec.ts Show resolved Hide resolved

packages/core/test/acceptance/query_spec.ts Outdated Show resolved Hide resolved

pullapprove bot requested a review from AndrewKushnir December 12, 2020 01:59

JoostK reviewed Dec 12, 2020

View reviewed changes

packages/core/src/util/array_utils.ts Outdated Show resolved Hide resolved

mhevery force-pushed the query_stable_notification branch 3 times, most recently from 29aa5b4 to 278dbce Compare December 14, 2020 23:56

AndrewKushnir reviewed Dec 15, 2020

View reviewed changes

pullapprove bot requested a review from AndrewKushnir December 15, 2020 00:47

JoostK reviewed Dec 15, 2020

View reviewed changes

mhevery added the action: merge The PR is ready for merge by the caretaker label Jan 13, 2021

IgorMinar approved these changes Jan 13, 2021

View reviewed changes

JoostK requested changes Jan 13, 2021

View reviewed changes

packages/compiler-cli/linker/src/file_linker/partial_linkers/partial_directive_linker_1.ts Outdated Show resolved Hide resolved

JoostK approved these changes Jan 14, 2021

View reviewed changes

mhevery removed request for petebacondarwin, atscott and alxhub January 14, 2021 20:38

mhevery added target: patch This PR is targeted for the next patch release and removed target: minor This PR is targeted for the next minor release labels Jan 14, 2021

mhevery added 2 commits January 14, 2021 13:24

refactor(core): Remove the need for explicit static query instruction

29cd0de

Because the query now has `flags` which specify the mode, the static query instruction can now be remove. It is simply normal query with `static` flag.

mhevery force-pushed the query_stable_notification branch from 4d81a44 to 29cd0de Compare January 14, 2021 21:28

AndrewKushnir added target: rc This PR is targeted for the next release-candidate and removed target: patch This PR is targeted for the next patch release labels Jan 14, 2021

AndrewKushnir closed this in e32b625 Jan 14, 2021

mhevery mentioned this pull request Feb 1, 2021

QueryList emits changes even when its items are not changed #35807

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): `QueryList` does not fire changes if the underlying list did not change. #40091

fix(core): `QueryList` does not fire changes if the underlying list did not change. #40091

mhevery commented Dec 11, 2020

mhevery commented Dec 11, 2020

mhevery commented Dec 11, 2020 •

edited

IgorMinar left a comment

AndrewKushnir left a comment •

edited

mhevery commented Dec 14, 2020

AndrewKushnir Dec 15, 2020

AndrewKushnir Dec 15, 2020

AndrewKushnir commented Dec 15, 2020

JoostK left a comment

JoostK Dec 15, 2020

mhevery Dec 15, 2020

JoostK Dec 15, 2020

mhevery commented Dec 15, 2020

AndrewKushnir commented Dec 16, 2020

mhevery commented Jan 13, 2021 •

edited

mary-poppins commented Jan 13, 2021

IgorMinar left a comment

mary-poppins commented Jan 13, 2021

mary-poppins commented Jan 13, 2021

mary-poppins commented Jan 14, 2021

mary-poppins commented Jan 14, 2021

AndrewKushnir commented Jan 14, 2021

JoostK commented Jan 14, 2021

AndrewKushnir commented Jan 14, 2021

angular-automatic-lock-bot bot commented Feb 14, 2021

		(this.changes as EventEmitter<any>).complete();
		(this.changes as EventEmitter<any>).unsubscribe();

fix(core): QueryList does not fire changes if the underlying list did not change. #40091

fix(core): QueryList does not fire changes if the underlying list did not change. #40091

Conversation

mhevery commented Dec 11, 2020

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

mhevery commented Dec 11, 2020

mhevery commented Dec 11, 2020 • edited

IgorMinar left a comment

Choose a reason for hiding this comment

AndrewKushnir left a comment • edited

Choose a reason for hiding this comment

mhevery commented Dec 14, 2020

AndrewKushnir Dec 15, 2020

Choose a reason for hiding this comment

AndrewKushnir Dec 15, 2020

Choose a reason for hiding this comment

AndrewKushnir commented Dec 15, 2020

JoostK left a comment

Choose a reason for hiding this comment

JoostK Dec 15, 2020

Choose a reason for hiding this comment

mhevery Dec 15, 2020

Choose a reason for hiding this comment

JoostK Dec 15, 2020

Choose a reason for hiding this comment

mhevery commented Dec 15, 2020

AndrewKushnir commented Dec 16, 2020

mhevery commented Jan 13, 2021 • edited

mary-poppins commented Jan 13, 2021

IgorMinar left a comment

Choose a reason for hiding this comment

mary-poppins commented Jan 13, 2021

mary-poppins commented Jan 13, 2021

mary-poppins commented Jan 14, 2021

mary-poppins commented Jan 14, 2021

AndrewKushnir commented Jan 14, 2021

JoostK commented Jan 14, 2021

AndrewKushnir commented Jan 14, 2021

angular-automatic-lock-bot bot commented Feb 14, 2021

fix(core): `QueryList` does not fire changes if the underlying list did not change. #40091

fix(core): `QueryList` does not fire changes if the underlying list did not change. #40091

mhevery commented Dec 11, 2020 •

edited

AndrewKushnir left a comment •

edited

mhevery commented Jan 13, 2021 •

edited