Sitelet https://web.archive.org/web/20210111220450/https://github.com/angular/angular/pull/40091
Skip to content

/ angular

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 3 commits into
base: master
from
Open

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

wants to merge 3 commits into from
+555 −280

Conversation

@mhevery
Copy link
Member

@mhevery mhevery commented Dec 11, 2020

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:

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
@google-cla google-cla bot added the cla: yes label Dec 11, 2020
@ngbot ngbot bot added this to the Backlog milestone Dec 11, 2020
@mhevery
Copy link
Member Author

@mhevery mhevery commented Dec 11, 2020

presubmit
@mhevery mhevery force-pushed the mhevery:query_stable_notification branch 2 times, most recently from 77480e0 to f02babb Dec 11, 2020
@mhevery
Copy link
Member Author

@mhevery mhevery commented Dec 11, 2020 •
edited

presubmit ( deflake )
@mhevery mhevery force-pushed the mhevery:query_stable_notification branch 2 times, most recently from 2cd900f to 266fa09 Dec 11, 2020
@IgorMinar
IgorMinar requested changes Dec 12, 2020
View changes
Copy link
Member

@IgorMinar IgorMinar left a comment

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 Outdated Show resolved Hide resolved
packages/core/src/util/array_utils.ts Outdated Show resolved Hide resolved
packages/core/src/util/array_utils.ts Outdated Show resolved Hide resolved
packages/core/test/util/array_utils_spec.ts Outdated Show resolved Hide resolved
packages/core/test/util/array_utils_spec.ts Outdated Show resolved Hide resolved
@pullapprove pullapprove bot requested a review from IgorMinar Dec 12, 2020
@AndrewKushnir
AndrewKushnir approved these changes Dec 12, 2020
View changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment •
edited

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 Outdated Show resolved Hide resolved
packages/core/test/acceptance/query_spec.ts Outdated Show resolved Hide resolved
packages/core/test/acceptance/query_spec.ts Outdated Show resolved Hide resolved
@pullapprove pullapprove bot requested a review from AndrewKushnir Dec 12, 2020
@JoostK
JoostK reviewed Dec 12, 2020
View changes
packages/core/src/util/array_utils.ts Outdated Show resolved Hide resolved
@mhevery
Copy link
Member Author

@mhevery mhevery commented Dec 14, 2020

@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.
@mhevery mhevery force-pushed the mhevery:query_stable_notification branch 3 times, most recently from 29aa5b4 to 278dbce Dec 14, 2020
@AndrewKushnir
AndrewKushnir reviewed Dec 15, 2020
View changes
packages/core/src/linker/query_list.ts Outdated
(this.changes as EventEmitter<any>).complete();
(this.changes as EventEmitter<any>).unsubscribe();
Comment on lines 203 to 186

This comment has been minimized.

Sign in to view
@AndrewKushnir

AndrewKushnir Dec 15, 2020
Contributor

We should probably do that for both _changesDeprecated and _changesStrict event emitters (in case both emitters were used in an app).
packages/core/src/linker/query_list.ts Outdated
(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];
Comment on lines 178 to 180

This comment has been minimized.

Sign in to view
@AndrewKushnir

AndrewKushnir Dec 15, 2020
Contributor

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 Outdated Show resolved Hide resolved
@pullapprove pullapprove bot requested a review from AndrewKushnir Dec 15, 2020
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Dec 15, 2020

FYI, started global presubmit.
@JoostK
JoostK reviewed Dec 15, 2020
View changes
Copy link
Member

@JoostK JoostK left a comment

I'd personally mark this for minor and not patch, since it introduces new API.
packages/core/src/linker/query_list.ts Outdated
@@ -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

This comment has been minimized.

Sign in to view
@JoostK

JoostK Dec 15, 2020
Member

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 comment has been minimized.

Sign in to view
@mhevery

mhevery Dec 15, 2020
Author Member

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

This comment has been minimized.

Sign in to view
@JoostK

JoostK Dec 15, 2020
Member

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 Outdated Show resolved Hide resolved
@mhevery
Copy link
Member Author

@mhevery mhevery commented Dec 15, 2020

presubmit
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Dec 16, 2020

Global presubmit #2 (using Ivy).
@jelbourn
jelbourn approved these changes Jan 7, 2021
View changes
Copy link
Member

@jelbourn jelbourn left a comment •
edited

LGTM

Reviewed-for: public-api
packages/core/src/linker/query_list.ts Outdated Show resolved Hide resolved
@mhevery mhevery force-pushed the mhevery:query_stable_notification branch from 6355a35 to 625551f Jan 7, 2021
@IgorMinar
IgorMinar reviewed Jan 7, 2021
View changes
Copy link
Member

@IgorMinar IgorMinar left a comment

as discussed on slack, let's go with the @ContentChildren('foo', {strictChangeEmit: true}) foos!: QueryList<...>;
@pullapprove pullapprove bot requested a review from IgorMinar Jan 7, 2021
@mhevery mhevery force-pushed the mhevery:query_stable_notification branch 6 times, most recently from 6ff6cac to acf73d7 Jan 8, 2021
@AndrewKushnir
AndrewKushnir reviewed Jan 9, 2021
View changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Thanks for the fix @mhevery 👍 Just left a few comments.
goldens/public-api/core/core.d.ts Outdated Show resolved Hide resolved
packages/compiler/src/render3/view/compiler.ts Outdated Show resolved Hide resolved
packages/core/src/linker/element_ref.ts Outdated Show resolved Hide resolved
packages/core/src/linker/query_list.ts Show resolved Hide resolved
packages/core/src/linker/query_list.ts Outdated Show resolved Hide resolved
packages/core/src/linker/query_list.ts Outdated Show resolved Hide resolved
packages/core/src/linker/query_list.ts Show resolved Hide resolved
packages/core/src/linker/query_list.ts Show resolved Hide resolved
packages/core/src/render3/interfaces/query.ts Outdated Show resolved Hide resolved
packages/core/src/render3/query.ts Outdated Show resolved Hide resolved
@axferch89
axferch89 approved these changes Jan 9, 2021
View changes
@axferch89
axferch89 approved these changes Jan 9, 2021
View changes
@JoostK
JoostK reviewed Jan 10, 2021
View changes
...ler_compliance/components_and_directives/queries/static_content_query.js Outdated Show resolved Hide resolved
packages/core/src/linker/query_list.ts Outdated Show resolved Hide resolved
packages/core/src/linker/query_list.ts Outdated Show resolved Hide resolved
packages/core/src/render3/query.ts Outdated Show resolved Hide resolved
packages/core/src/render3/query.ts Outdated Show resolved Hide resolved
packages/core/test/render3/jit/declare_component_spec.ts Outdated Show resolved Hide resolved
packages/core/test/render3/jit/declare_directive_spec.ts Outdated Show resolved Hide resolved
packages/core/test/render3/jit/declare_directive_spec.ts Outdated Show resolved Hide resolved
packages/core/test/render3/jit/declare_directive_spec.ts Outdated Show resolved Hide resolved
packages/core/test/render3/jit/declare_directive_spec.ts Outdated Show resolved Hide resolved
@mary-poppins
Copy link

@mary-poppins mary-poppins commented Jan 11, 2021

You can preview 1d4e830 at https://pr40091-1d4e830.ngbuilds.io/.
You can preview f02babb at https://pr40091-f02babb.ngbuilds.io/.
You can preview 2cd900f at https://pr40091-2cd900f.ngbuilds.io/.
You can preview 266fa09 at https://pr40091-266fa09.ngbuilds.io/.
You can preview 8a377c1 at https://pr40091-8a377c1.ngbuilds.io/.
You can preview 278dbce at https://pr40091-278dbce.ngbuilds.io/.
You can preview d7c49d4 at https://pr40091-d7c49d4.ngbuilds.io/.
You can preview ce929a5 at https://pr40091-ce929a5.ngbuilds.io/.
You can preview bdb1b09 at https://pr40091-bdb1b09.ngbuilds.io/.
You can preview 625551f at https://pr40091-625551f.ngbuilds.io/.
You can preview e4dcceb at https://pr40091-e4dcceb.ngbuilds.io/.
You can preview f777d24 at https://pr40091-f777d24.ngbuilds.io/.
You can preview 1e8a1d8 at https://pr40091-1e8a1d8.ngbuilds.io/.
You can preview 6ff6cac at https://pr40091-6ff6cac.ngbuilds.io/.
You can preview acf73d7 at https://pr40091-acf73d7.ngbuilds.io/.
@IgorMinar
IgorMinar requested changes Jan 11, 2021
View changes
Copy link
Member

@IgorMinar IgorMinar left a comment

I like the new api name emitDistinctChangesOnly a lot more than the previous options. Can you please rerun the api guard to get the PR into a green state? thanks!
@mhevery mhevery force-pushed the mhevery:query_stable_notification branch from acf73d7 to 77eb52b Jan 11, 2021
@mary-poppins
Copy link

@mary-poppins mary-poppins commented Jan 11, 2021

You can preview 77eb52b at https://pr40091-77eb52b.ngbuilds.io/.
@mhevery mhevery force-pushed the mhevery:query_stable_notification branch from 77eb52b to 847658a Jan 11, 2021
@mary-poppins
Copy link

@mary-poppins mary-poppins commented Jan 11, 2021

You can preview 847658a at https://pr40091-847658a.ngbuilds.io/.
mhevery added 2 commits Dec 10, 2020
@mhevery
fix(core): `QueryList` does not fire changes if the underlying list d…
Loading status checks…
cee489e 
…id not change.

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.)
@mhevery
refactor(core): Remove the need for explicit static query instruction
Loading status checks…
f88eb1d 
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
Copy link
Member Author

@mhevery mhevery commented Jan 11, 2021

presubmit
@AndrewKushnir
AndrewKushnir approved these changes Jan 11, 2021
View changes
packages/compiler/src/render3/view/compiler.ts

none = 0,
Comment on lines +455 to +456

This comment has been minimized.

Sign in to view
@AndrewKushnir

AndrewKushnir Jan 11, 2021
Contributor

Small indentation update (rm line before none, add one after):

Suggested change
none = 0,
none = 0,

This comment has been minimized.

Sign in to view
@mhevery

mhevery Jan 11, 2021
Author Member

Why line after? I think it is easier to read. Other places have line breaks like that as well.

This comment has been minimized.

Sign in to view
@AndrewKushnir

AndrewKushnir Jan 11, 2021
Contributor

It just looks like the comment is not "attached" to the none = 0 string (since there is a line in between a comment and an expression).

This comment has been minimized.

Sign in to view
@mhevery

mhevery Jan 11, 2021
Author Member

yes, that one (break between comment and declaration) I removed. But you are suggesting to remove the empty line after. That one I think should stay.
packages/core/src/linker/query_list.ts Show resolved Hide resolved
@AndrewKushnir AndrewKushnir requested a review from IgorMinar Jan 11, 2021
@mhevery mhevery force-pushed the mhevery:query_stable_notification branch from 847658a to c469e9a Jan 11, 2021
@mary-poppins
Copy link

@mary-poppins mary-poppins commented Jan 11, 2021

You can preview c469e9a at https://pr40091-c469e9a.ngbuilds.io/.
@mhevery
fixup! refactor(core): Remove the need for explicit static query inst…
Loading status checks…
9c15a80 
…ruction
@mhevery mhevery force-pushed the mhevery:query_stable_notification branch from c469e9a to 9c15a80 Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoostK JoostK

@AndrewKushnir AndrewKushnir

@jelbourn jelbourn

@axferch89 axferch89

@IgorMinar IgorMinar

@atscott atscott

@alxhub alxhub

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
aio: preview cla: yes comp: core comp: docs/api target: patch
Projects
None yet
Milestone
Backlog
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
@mhevery @AndrewKushnir @jelbourn @mary-poppins @JoostK @IgorMinar @axferch89
You can’t perform that action at this time.