Skip to content

Commit

Permalink
fix(core): Dirty views that are reattached should get refreshed durin…
Browse files Browse the repository at this point in the history
…g CD

Another fix related to angular#52928. When a view has the `Dirty` flag and is
reattached, we should ensure that it is reached and refreshed during
the next change detection run from above.

This commit does not actually fix the reproduction reported in angular#52928. However,
that's arguably not something we should try to address because there's
no clear indication that a `CheckAlways` view needs to be refreshed
during change detection (aside from it being in creation mode/first
update pass like the reported issue).
  • Loading branch information
atscott committed Dec 9, 2023
1 parent e3a6bf9 commit 3e27037
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 14 deletions.
6 changes: 6 additions & 0 deletions packages/core/src/render3/util/view_utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,12 @@ export function requiresRefreshOrTraversal(lView: LView) {
* parents above.
*/
export function updateAncestorTraversalFlagsOnAttach(lView: LView) {
// When we attach a view that's marked `Dirty`, we should ensure that it is reached during the
// next CD traversal so we add the `RefreshView` flag and mark ancestors accordingly.
if (lView[FLAGS] & LViewFlags.Dirty) {
lView[FLAGS] |= LViewFlags.RefreshView;
}

if (!requiresRefreshOrTraversal(lView)) {
return;
}
Expand Down
36 changes: 22 additions & 14 deletions packages/core/test/acceptance/change_detection_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -187,54 +187,62 @@ describe('change detection', () => {
@Component({
selector: `test-cmpt`,
template: `{{counter}}|<ng-template #vc></ng-template>`,
changeDetection: ChangeDetectionStrategy.OnPush
changeDetection: ChangeDetectionStrategy.OnPush,
standalone: true,
})
class TestCmpt {
counter = 0;
@ViewChild('vc', {read: ViewContainerRef}) vcRef!: ViewContainerRef;

constructor() {}

createComponentView<T>(cmptType: Type<T>): ComponentRef<T> {
return this.vcRef.createComponent(cmptType);
}
}

@Component({
selector: 'dynamic-cmpt',
template: `dynamic`,
template: `dynamic|{{binding}}`,
standalone: true,
changeDetection: ChangeDetectionStrategy.OnPush
})
class DynamicCmpt {
@Input() binding = 'binding';
}

@NgModule({declarations: [DynamicCmpt]})
class DynamicModule {
}

TestBed.configureTestingModule({imports: [DynamicModule], declarations: [TestCmpt]});

const fixture = TestBed.createComponent(TestCmpt);

// initial CD to have query results
// NOTE: we call change detection without checkNoChanges to have clearer picture
fixture.detectChanges(false);
expect(fixture.nativeElement).toHaveText('0|');

// insert a dynamic component
// insert a dynamic component, but do not specifically mark parent dirty
// (dynamic components with OnPush flag are created with the `Dirty` flag)
const dynamicCmptRef = fixture.componentInstance.createComponentView(DynamicCmpt);
fixture.detectChanges(false);
expect(fixture.nativeElement).toHaveText('0|dynamic');
expect(fixture.nativeElement).toHaveText('0|dynamic|binding');

// update model in the OnPush component - should not update UI
fixture.componentInstance.counter = 1;
fixture.detectChanges(false);
expect(fixture.nativeElement).toHaveText('0|dynamic');
expect(fixture.nativeElement).toHaveText('0|dynamic|binding');

// now mark the dynamically inserted component as dirty
dynamicCmptRef.changeDetectorRef.markForCheck();
fixture.detectChanges(false);
expect(fixture.nativeElement).toHaveText('1|dynamic');
expect(fixture.nativeElement).toHaveText('1|dynamic|binding');

// Update, mark for check, and detach before change detection, should not update
dynamicCmptRef.setInput('binding', 'updatedBinding');
dynamicCmptRef.changeDetectorRef.markForCheck();
dynamicCmptRef.changeDetectorRef.detach();
fixture.detectChanges(false);
expect(fixture.nativeElement).toHaveText('1|dynamic|binding');

// reattaching and run CD from the top should update
dynamicCmptRef.changeDetectorRef.reattach();
fixture.detectChanges(false);
expect(fixture.nativeElement).toHaveText('1|dynamic|updatedBinding');
});

it('should support re-enterant change detection', () => {
Expand Down

0 comments on commit 3e27037

Please sign in to comment.