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
fix(core): Switch emitDistinctChangesOnlyDefaultValue
to true
#41121
fix(core): Switch emitDistinctChangesOnlyDefaultValue
to true
#41121
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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:
- packages/compiler/src/render3/partial/api.ts#L240-L242
- packages/core/src/linker/query_list.ts#L65-L66
- packages/core/src/metadata/di.ts#L151-L152
- packages/core/src/metadata/di.ts#L290-L291
@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/core/src/metadata/di.ts
Outdated
// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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. |
d33e8e9
to
f0f1139
Compare
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there plans to deprecate this, or do we want to keep this around?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
60cde89
to
e976dd4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
e976dd4
to
dd0ea2f
Compare
You can preview d33e8e9 at https://pr41121-d33e8e9.ngbuilds.io/. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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).
dd0ea2f
to
1054319
Compare
You can preview 1054319 at https://pr41121-1054319.ngbuilds.io/. |
0e32b8c
to
28e117c
Compare
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`.
28e117c
to
fa29891
Compare
You can preview fa29891 at https://pr41121-fa29891.ngbuilds.io/. |
CARETAKER: cl/361617255 must land before this change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
You can preview da5c248 at https://pr41121-da5c248.ngbuilds.io/. |
You can preview a6ba19b at https://pr41121-a6ba19b.ngbuilds.io/. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm for public api change
Reviewed-for: public-api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Reviewed-for: public-api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Reviewed-for: docs-packaging-and-releasing
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information