-
Notifications
You must be signed in to change notification settings - Fork 25.8k
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): When using setInput, mark view dirty in same was as `markF… #49711
Conversation
bc5ffb9
to
c1f001c
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.
Seems like a good direction, will need test + golden symbols update.
Ping me when it is ready for review - I'm curious what code we are going to pull and how it is going to impact size
…orCheck` `ComponentRef.setInput` internally calls `markDirtyIfOnPush` which only marks the given view as dirty but does not mark parents dirty like `ChangeDetectorRef.markForCheck` would. https://github.com/angular/angular/blob/f071224720f8affb97fd32fb5aeaa13155b13693/packages/core/src/render3/instructions/shared.ts#L1018-L1024 `markDirtyIfOnPush` has an assumption that it’s being called from the parent’s template. That is, we don’t need to mark dirty to the root, because we’ve already traversed down to it. The function used to only be called during template execution for input bindings but was added to `setInput` later. It's not a good fit because it means that if you are responding to events such as an emit from an `Observable` and call `setInput`, the view of your `ComponentRef` won't necessarily get checked when change detection runs next. If this lives inside some `OnPush` component tree that's not already dirty, it only gets refreshed if you also call `ChangeDetectorRef.markForCheck` in the host component (because it will be "shielded" be a non-dirty parent).
c1f001c
to
8eb672f
Compare
This PR was merged into the repository by commit a4e749f. |
Hey, thanks for fixing this issue in v16. As of now we've got this exact update problem in our application. As we are still a month away from the scheduled release of v16 and further away from upgrading our project to the newest version, I'd like to request to backport this fix to v15.1 and v15.2 respectively. I've created an example using an overlay having a projected component into it. Inputs for the projected component are set via the Example: Hover on the 'Hover me' text to see if the overlay component gets rendered properly and enable and disable markForCheck to activate the workaround. Stackblitz example using v15.2.6 having buggy behavior: Stackblitz example using v16.0.0-next.7 validating this fix as being effective: Please backport this fix to v15.1 and 15.2. Thanks! |
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. |
…orCheck`
ComponentRef.setInput
internally callsmarkDirtyIfOnPush
which only marks the given view as dirty but does not mark parents dirty likeChangeDetectorRef.markForCheck
would.angular/packages/core/src/render3/instructions/shared.ts
Lines 1018 to 1024 in f071224
markDirtyIfOnPush
has an assumption that it’s being called from the parent’s template. That is, we don’t need to mark dirty to the root, because we’ve already traversed down to it. The function used to only be called during template execution for input bindings but was added tosetInput
later. It's not a good fit because it means that if you are responding to events such as an emit from anObservable
and callsetInput
, the view of yourComponentRef
won't necessarily get checked when change detection runs next. If this lives inside someOnPush
component tree that's not already dirty, it only gets refreshed if you also callChangeDetectorRef.markForCheck
in the host component (because it will be "shielded" be a non-dirty parent).