fix(core): Switch `emitDistinctChangesOnlyDefaultValue` to true by mhevery · Pull Request #41121 · angular/angular

mhevery · 2021-03-08T19:23:48Z

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 · 2021-03-08T19:36:32Z

presubmit

AndrewKushnir

@mhevery thanks for switching the default value!

It looks like there are few more places where the comments (that refer to v12) should be updated:

@mhevery should we mark this PR as a breaking change and add the "BREAKING CHANGE" note to the commit message as well? The previous PR where this change was introduced was backwards-compatible, but flipping the default would trigger a new codepath for all apps (that might potentially be breaking).

Also it looks like there is one legit failing test on CI, could you please have a look?

Thank you.

packages/compiler/src/core.ts

AndrewKushnir · 2021-03-08T19:54:02Z

packages/core/src/metadata/di.ts

Suggested change

// 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

// Stores the default value of `emitDistinctChangesOnly` when the `emitDistinctChangesOnly` is not

// explicitly set.

JoostK · 2021-03-08T21:03:21Z

packages/compiler/src/render3/partial/api.ts

Are there plans to deprecate this, or do we want to keep this around?

I think we should delete this global flag after everything lands and things are quite. Deleting the flag now, would make rollback difficult if we run into issues.

I see this comment may have been a bit out of place; I thought this was the public facing metadata object for ContentQuery/ViewQuery; I meant the deprecation of emitDistinctChangesOnly as it can be configured by developers for ContentQuery/ViewQuery. Do we want to keep supporting it being set to false? My vote would be to deprecate the flag now, so that it's clear that using emitDistinctChangesOnly: false to mitigate the breaking change is only to buy the developer some extra time in their migration, and that the option will be going away in the future.

AndrewKushnir

Thanks for addressing the feedback @mhevery 👍

I think we can update the commit message to add the "BREAKING CHANGE" note above the commit description, so that it goes into the breaking changes section of the CHANGELOG with all the details (that'd help migrate apps over):

fix(core): Switch `emitDistinctChangesOnlyDefaultValue` to true

BREAKING CHANGE:

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.

...

If I understand correctly, @JoostK proposes to add the @deprecated JSDoc annotation to the emitDistinctChangesOnly flag of the query decorator arguments, so that we can remove it altogether later (in v13 or v14). For completeness, we can probably do that in the same commit and extend the "BREAKING CHANGE" note to mention this deprecation.

It looks like CI has some failing tests as well, which are related to the changes in this PR.

Thank you.

mary-poppins · 2021-03-09T01:26:01Z

You can preview d33e8e9 at https://pr41121-d33e8e9.ngbuilds.io/.
You can preview f0f1139 at https://pr41121-f0f1139.ngbuilds.io/.
You can preview 60cde89 at https://pr41121-60cde89.ngbuilds.io/.
You can preview e976dd4 at https://pr41121-e976dd4.ngbuilds.io/.
You can preview dd0ea2f at https://pr41121-dd0ea2f.ngbuilds.io/.

AndrewKushnir

@mhevery I've just added a couple more comments, please see below.

I think we should also add a deprecation notice to this page: https://angular.io/guide/deprecations#angularcore (the aio/content/guide/deprecations.md file).

packages/core/src/metadata/di.ts

mary-poppins · 2021-03-10T19:40:59Z

You can preview 1054319 at https://pr41121-1054319.ngbuilds.io/.

@ContentChildren

BREAKING CHANGE: Switching default of `emitDistinctChangesOnlyDefaultValue` which changes the default behavior and may cause some applications which rely on the incorrect behavior to fail. `emitDistinctChangesOnly` flag has also been deprecated and will be removed in a future major release. 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`.

mary-poppins · 2021-03-10T20:28:26Z

You can preview fa29891 at https://pr41121-fa29891.ngbuilds.io/.

mhevery · 2021-03-10T20:41:06Z

CARETAKER: cl/361617255 must land before this change

AndrewKushnir

Looks great, thanks @mhevery 👍

Couple quick comments:

we should probably include the emitDistinctChangesOnly deprecation to the https://angular.io/guide/deprecations#angularcore page as well (aio/content/guide/deprecations.md)
also include a note on the deprecation into the commit message

Thank you.

mhevery · 2021-03-10T20:58:50Z

presubmit

mary-poppins · 2021-03-10T22:18:24Z

You can preview da5c248 at https://pr41121-da5c248.ngbuilds.io/.

packages/core/test/acceptance/query_spec.ts

packages/compiler/src/render3/partial/directive.ts

mhevery · 2021-03-11T00:09:49Z

@joost see a6ba19b

mary-poppins · 2021-03-11T00:18:54Z

You can preview a6ba19b at https://pr41121-a6ba19b.ngbuilds.io/.

IgorMinar

Lgtm for public api change

Reviewed-for: public-api

jelbourn

LGTM

Reviewed-for: public-api

jelbourn

LGTM

Reviewed-for: docs-packaging-and-releasing

angular-automatic-lock-bot · 2021-04-12T16:11:25Z

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 Mar 8, 2021

pullapprove bot requested a review from AndrewKushnir March 8, 2021 19:24

mhevery added the target: major This PR is targeted for the next major release label Mar 8, 2021

AndrewKushnir reviewed Mar 8, 2021

View reviewed changes

mhevery added the breaking changes label Mar 8, 2021

mhevery force-pushed the emitDistinctChangesOnlyDefaultValue branch from d33e8e9 to f0f1139 Compare March 8, 2021 20:57

pullapprove bot requested a review from JoostK March 8, 2021 20:57

JoostK reviewed Mar 8, 2021

View reviewed changes

mhevery force-pushed the emitDistinctChangesOnlyDefaultValue branch 2 times, most recently from 60cde89 to e976dd4 Compare March 8, 2021 23:05

AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime labels Mar 8, 2021

ngbot bot modified the milestone: Backlog Mar 8, 2021

AndrewKushnir reviewed Mar 8, 2021

View reviewed changes

mhevery force-pushed the emitDistinctChangesOnlyDefaultValue branch from e976dd4 to dd0ea2f Compare March 9, 2021 00:50

AndrewKushnir added the aio: preview label Mar 9, 2021

AndrewKushnir reviewed Mar 9, 2021

View reviewed changes

packages/core/src/metadata/di.ts Outdated Show resolved Hide resolved

packages/core/src/metadata/di.ts Outdated Show resolved Hide resolved

mhevery force-pushed the emitDistinctChangesOnlyDefaultValue branch from dd0ea2f to 1054319 Compare March 10, 2021 19:31

mhevery force-pushed the emitDistinctChangesOnlyDefaultValue branch 2 times, most recently from 0e32b8c to 28e117c Compare March 10, 2021 20:17

mhevery force-pushed the emitDistinctChangesOnlyDefaultValue branch from 28e117c to fa29891 Compare March 10, 2021 20:19

mhevery added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Mar 10, 2021

ngbot bot added the action: merge The PR is ready for merge by the caretaker label Mar 10, 2021

mhevery removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Mar 10, 2021

AndrewKushnir approved these changes Mar 10, 2021

View reviewed changes

AndrewKushnir added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Mar 10, 2021

fixup! fix(core): Switch emitDistinctChangesOnlyDefaultValue to true

da5c248

pullapprove bot requested a review from IgorMinar March 10, 2021 22:09

mhevery removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Mar 10, 2021

JoostK approved these changes Mar 10, 2021

View reviewed changes

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

packages/compiler/src/render3/partial/directive.ts Outdated Show resolved Hide resolved

fixup! fix(core): Switch emitDistinctChangesOnlyDefaultValue to true

a6ba19b

mhevery removed the request for review from IgorMinar March 11, 2021 00:10

pullapprove bot requested a review from IgorMinar March 11, 2021 00:10

mhevery requested review from jelbourn and removed request for IgorMinar March 11, 2021 00:26

IgorMinar approved these changes Mar 11, 2021

View reviewed changes

jelbourn approved these changes Mar 12, 2021

View reviewed changes

pullapprove bot requested a review from IgorMinar March 12, 2021 16:01

jelbourn approved these changes Mar 12, 2021

View reviewed changes

mhevery removed the request for review from IgorMinar March 12, 2021 17:39

jessicajaniuk closed this in 7096246 Mar 12, 2021

angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 12, 2021

-// 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
+// Stores the default value of `emitDistinctChangesOnly` when the `emitDistinctChangesOnly` is not
+// explicitly set.

Conversation

mhevery commented Mar 8, 2021

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

mhevery commented Mar 8, 2021

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AndrewKushnir Mar 8, 2021

Choose a reason for hiding this comment

Uh oh!

JoostK Mar 8, 2021

Choose a reason for hiding this comment

Uh oh!

mhevery Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoostK Mar 8, 2021

Choose a reason for hiding this comment

Uh oh!

mhevery Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Mar 9, 2021

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mary-poppins commented Mar 10, 2021

Uh oh!

mary-poppins commented Mar 10, 2021

Uh oh!

mhevery commented Mar 10, 2021

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

mhevery commented Mar 10, 2021

Uh oh!

mary-poppins commented Mar 10, 2021

Uh oh!

Uh oh!

Uh oh!

mhevery commented Mar 11, 2021

Uh oh!

mary-poppins commented Mar 11, 2021

Uh oh!

IgorMinar left a comment

Choose a reason for hiding this comment

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Apr 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mhevery Mar 8, 2021 •

edited

Loading