From 606de5166e33affd7bd53d1ad86c11afa3cdbe21 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Fri, 17 Nov 2023 13:34:45 -0800 Subject: [PATCH] refactor(core): newly created and any dirty views should get refreshed during CD (#53022) 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. In addition, when a view is created and attached, we should ensure that it is reached and refreshed during change detection. This can happen if the view is created and attached outside a change run or when it is created and attached after its insertion view was already checked. In both cases, we should ensure that the view is reached and refreshed during either the current change detection or the next one (if change detection is not already running). We can achieve this by creating all views with the `Dirty` flag set. However, this does happen to be a breaking change in some scenarios. The one identified internally was actually depending on change detection _not_ running immediately because it relied on an input value that was set using `ngModel`. Because `ngModel` sets its value in a `Promise`, it is not available until the _next_ change detection cycle. Ensuring created views run in the current change change detection will result in different behavior in this case. Making option the default is the solution to #52928. That will have to wait for a major version. PR Close #53022 --- packages/core/src/change_detection/flags.ts | 17 ++++++++ packages/core/src/core_private_export.ts | 1 + packages/core/src/render3/component_ref.ts | 12 +++--- .../core/src/render3/instructions/shared.ts | 3 +- packages/core/src/render3/util/view_utils.ts | 7 ++++ .../test/acceptance/change_detection_spec.ts | 41 ++++++++++++------- .../bundling/defer/bundle.golden_symbols.json | 6 +++ .../forms_reactive/bundle.golden_symbols.json | 3 ++ .../bundle.golden_symbols.json | 3 ++ .../router/bundle.golden_symbols.json | 3 ++ .../bundling/todo/bundle.golden_symbols.json | 3 ++ .../change_detection_integration_spec.ts | 21 +++++----- 12 files changed, 87 insertions(+), 33 deletions(-) create mode 100644 packages/core/src/change_detection/flags.ts diff --git a/packages/core/src/change_detection/flags.ts b/packages/core/src/change_detection/flags.ts new file mode 100644 index 00000000000..0c816a7573b --- /dev/null +++ b/packages/core/src/change_detection/flags.ts @@ -0,0 +1,17 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +// TODO(atscott): flip default internally ASAP and externally for v18 (#52928) +let _ensureDirtyViewsAreAlwaysReachable = false; + +export function getEnsureDirtyViewsAreAlwaysReachable() { + return _ensureDirtyViewsAreAlwaysReachable; +} +export function setEnsureDirtyViewsAreAlwaysReachable(v: boolean) { + _ensureDirtyViewsAreAlwaysReachable = v; +} diff --git a/packages/core/src/core_private_export.ts b/packages/core/src/core_private_export.ts index a6a4358734d..4bcbf0bbe41 100644 --- a/packages/core/src/core_private_export.ts +++ b/packages/core/src/core_private_export.ts @@ -11,6 +11,7 @@ export {whenStable as ɵwhenStable} from './application/application_ref'; export {IMAGE_CONFIG as ɵIMAGE_CONFIG, IMAGE_CONFIG_DEFAULTS as ɵIMAGE_CONFIG_DEFAULTS, ImageConfig as ɵImageConfig} from './application/application_tokens'; export {internalCreateApplication as ɵinternalCreateApplication} from './application/create_application'; export {defaultIterableDiffers as ɵdefaultIterableDiffers, defaultKeyValueDiffers as ɵdefaultKeyValueDiffers} from './change_detection/change_detection'; +export {getEnsureDirtyViewsAreAlwaysReachable as ɵgetEnsureDirtyViewsAreAlwaysReachable, setEnsureDirtyViewsAreAlwaysReachable as ɵsetEnsureDirtyViewsAreAlwaysReachable} from './change_detection/flags'; export {Console as ɵConsole} from './console'; export {DeferBlockDetails as ɵDeferBlockDetails, getDeferBlocks as ɵgetDeferBlocks} from './defer/discovery'; export {renderDeferBlockState as ɵrenderDeferBlockState, triggerResourceLoading as ɵtriggerResourceLoading} from './defer/instructions'; diff --git a/packages/core/src/render3/component_ref.ts b/packages/core/src/render3/component_ref.ts index c5ee9dc3b2b..5a30f134c24 100644 --- a/packages/core/src/render3/component_ref.ts +++ b/packages/core/src/render3/component_ref.ts @@ -220,12 +220,12 @@ export class ComponentFactory extends AbstractComponentFactory { hostRenderer, rootSelectorOrNode, this.componentDef.encapsulation, rootViewInjector) : createElementNode(hostRenderer, elementName, getNamespace(elementName)); - // Signal components use the granular "RefreshView" for change detection - const signalFlags = (LViewFlags.SignalView | LViewFlags.IsRoot); - // Non-signal components use the traditional "CheckAlways or OnPush/Dirty" change detection - const nonSignalFlags = this.componentDef.onPush ? LViewFlags.Dirty | LViewFlags.IsRoot : - LViewFlags.CheckAlways | LViewFlags.IsRoot; - const rootFlags = this.componentDef.signals ? signalFlags : nonSignalFlags; + let rootFlags = LViewFlags.IsRoot; + if (this.componentDef.signals) { + rootFlags |= LViewFlags.SignalView; + } else if (!this.componentDef.onPush) { + rootFlags |= LViewFlags.CheckAlways; + } let hydrationInfo: DehydratedView|null = null; if (hostRNode !== null) { diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index 0ad5d165713..ab3bff9c1b7 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -94,7 +94,8 @@ export function createLView( hydrationInfo: DehydratedView|null): LView { const lView = tView.blueprint.slice() as LView; lView[HOST] = host; - lView[FLAGS] = flags | LViewFlags.CreationMode | LViewFlags.Attached | LViewFlags.FirstLViewPass; + lView[FLAGS] = flags | LViewFlags.CreationMode | LViewFlags.Attached | LViewFlags.FirstLViewPass | + LViewFlags.Dirty; if (embeddedViewInjector !== null || (parentLView && (parentLView[FLAGS] & LViewFlags.HasEmbeddedViewInjector))) { lView[FLAGS] |= LViewFlags.HasEmbeddedViewInjector; diff --git a/packages/core/src/render3/util/view_utils.ts b/packages/core/src/render3/util/view_utils.ts index 59f2656e7ba..2648ae98566 100644 --- a/packages/core/src/render3/util/view_utils.ts +++ b/packages/core/src/render3/util/view_utils.ts @@ -6,6 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ +import {getEnsureDirtyViewsAreAlwaysReachable} from '../../change_detection/flags'; import {RuntimeError, RuntimeErrorCode} from '../../errors'; import {assertDefined, assertGreaterThan, assertGreaterThanOrEqual, assertIndexInRange, assertLessThan} from '../../util/assert'; import {assertTNode, assertTNodeForLView} from '../assert'; @@ -206,6 +207,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 && getEnsureDirtyViewsAreAlwaysReachable()) { + lView[FLAGS] |= LViewFlags.RefreshView; + } + if (!requiresRefreshOrTraversal(lView)) { return; } diff --git a/packages/core/test/acceptance/change_detection_spec.ts b/packages/core/test/acceptance/change_detection_spec.ts index 77efadee300..16ba8f3aaca 100644 --- a/packages/core/test/acceptance/change_detection_spec.ts +++ b/packages/core/test/acceptance/change_detection_spec.ts @@ -8,7 +8,7 @@ import {CommonModule} from '@angular/common'; -import {ApplicationRef, ChangeDetectionStrategy, ChangeDetectorRef, Component, ComponentRef, createComponent, Directive, DoCheck, EmbeddedViewRef, ErrorHandler, EventEmitter, inject, Input, NgModule, OnInit, Output, QueryList, TemplateRef, Type, ViewChild, ViewChildren, ViewContainerRef} from '@angular/core'; +import {ApplicationRef, ChangeDetectionStrategy, ChangeDetectorRef, Component, ComponentRef, Directive, DoCheck, EmbeddedViewRef, ErrorHandler, EventEmitter, inject, Input, NgModule, OnInit, Output, QueryList, TemplateRef, Type, ViewChild, ViewChildren, ViewContainerRef, ɵgetEnsureDirtyViewsAreAlwaysReachable, ɵsetEnsureDirtyViewsAreAlwaysReachable} from '@angular/core'; import {ComponentFixture, fakeAsync, TestBed, tick} from '@angular/core/testing'; import {expect} from '@angular/platform-browser/testing/src/matchers'; import {BehaviorSubject} from 'rxjs'; @@ -184,17 +184,18 @@ describe('change detection', () => { describe('markForCheck', () => { it('should mark OnPush ancestor of dynamically created component views as dirty', () => { + const previous = ɵgetEnsureDirtyViewsAreAlwaysReachable(); + ɵsetEnsureDirtyViewsAreAlwaysReachable(true); @Component({ selector: `test-cmpt`, template: `{{counter}}|`, - changeDetection: ChangeDetectionStrategy.OnPush + changeDetection: ChangeDetectionStrategy.OnPush, + standalone: true, }) class TestCmpt { counter = 0; @ViewChild('vc', {read: ViewContainerRef}) vcRef!: ViewContainerRef; - constructor() {} - createComponentView(cmptType: Type): ComponentRef { return this.vcRef.createComponent(cmptType); } @@ -202,18 +203,14 @@ describe('change detection', () => { @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 @@ -221,20 +218,34 @@ describe('change detection', () => { 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'); + ɵsetEnsureDirtyViewsAreAlwaysReachable(previous); }); it('should support re-enterant change detection', () => { diff --git a/packages/core/test/bundling/defer/bundle.golden_symbols.json b/packages/core/test/bundling/defer/bundle.golden_symbols.json index 4b108157451..0ea35badcc3 100644 --- a/packages/core/test/bundling/defer/bundle.golden_symbols.json +++ b/packages/core/test/bundling/defer/bundle.golden_symbols.json @@ -533,6 +533,9 @@ { "name": "_enable_super_gross_mode_that_will_cause_bad_things" }, + { + "name": "_ensureDirtyViewsAreAlwaysReachable" + }, { "name": "_findMatchingDehydratedViewImpl" }, @@ -1334,6 +1337,9 @@ { "name": "init_fields" }, + { + "name": "init_flags" + }, { "name": "init_forward_ref" }, diff --git a/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json b/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json index 2597f240222..986f1c70789 100644 --- a/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json +++ b/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json @@ -638,6 +638,9 @@ { "name": "_enable_super_gross_mode_that_will_cause_bad_things" }, + { + "name": "_ensureDirtyViewsAreAlwaysReachable" + }, { "name": "_getInsertInFrontOfRNodeWithI18n" }, diff --git a/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json b/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json index 7441ab2f4b3..294827acbc2 100644 --- a/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json +++ b/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json @@ -629,6 +629,9 @@ { "name": "_enable_super_gross_mode_that_will_cause_bad_things" }, + { + "name": "_ensureDirtyViewsAreAlwaysReachable" + }, { "name": "_getInsertInFrontOfRNodeWithI18n" }, diff --git a/packages/core/test/bundling/router/bundle.golden_symbols.json b/packages/core/test/bundling/router/bundle.golden_symbols.json index 9e438ea37a2..d780911f106 100644 --- a/packages/core/test/bundling/router/bundle.golden_symbols.json +++ b/packages/core/test/bundling/router/bundle.golden_symbols.json @@ -863,6 +863,9 @@ { "name": "_enable_super_gross_mode_that_will_cause_bad_things" }, + { + "name": "_ensureDirtyViewsAreAlwaysReachable" + }, { "name": "_getInsertInFrontOfRNodeWithI18n" }, diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json index cc6a77eb512..5bc987d4e3e 100644 --- a/packages/core/test/bundling/todo/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json @@ -554,6 +554,9 @@ { "name": "_enable_super_gross_mode_that_will_cause_bad_things" }, + { + "name": "_ensureDirtyViewsAreAlwaysReachable" + }, { "name": "_getInsertInFrontOfRNodeWithI18n" }, diff --git a/packages/core/test/linker/change_detection_integration_spec.ts b/packages/core/test/linker/change_detection_integration_spec.ts index 0c98abe4ff6..5cc67f0629d 100644 --- a/packages/core/test/linker/change_detection_integration_spec.ts +++ b/packages/core/test/linker/change_detection_integration_spec.ts @@ -7,7 +7,7 @@ */ import {ResourceLoader} from '@angular/compiler'; -import {AfterContentChecked, AfterContentInit, AfterViewChecked, AfterViewInit, ChangeDetectionStrategy, ChangeDetectorRef, Component, ContentChild, DebugElement, Directive, DoCheck, EventEmitter, HostBinding, Injectable, Input, OnChanges, OnDestroy, OnInit, Output, Pipe, PipeTransform, Provider, RendererFactory2, RendererType2, SimpleChange, SimpleChanges, TemplateRef, Type, ViewChild, ViewContainerRef} from '@angular/core'; +import {AfterContentChecked, AfterContentInit, AfterViewChecked, AfterViewInit, ChangeDetectionStrategy, ChangeDetectorRef, Component, ContentChild, DebugElement, Directive, DoCheck, EventEmitter, HostBinding, Injectable, Input, OnChanges, OnDestroy, OnInit, Output, Pipe, PipeTransform, Provider, RendererFactory2, RendererType2, SimpleChange, SimpleChanges, TemplateRef, Type, ViewChild, ViewContainerRef, ɵgetEnsureDirtyViewsAreAlwaysReachable, ɵsetEnsureDirtyViewsAreAlwaysReachable} from '@angular/core'; import {ComponentFixture, fakeAsync, TestBed} from '@angular/core/testing'; import {By} from '@angular/platform-browser/src/dom/debug/by'; import {isTextNode} from '@angular/platform-browser/testing/src/browser_util'; @@ -1135,16 +1135,15 @@ describe(`ChangeDetection`, () => { expect(() => ctx.checkNoChanges()).toThrowError(errMsgRegExp); })); - it('should warn when the view has been created in a cd hook', fakeAsync(() => { - const ctx = createCompFixture('
{{ a }}
', TestData); - ctx.componentInstance.a = 1; - expect(() => ctx.detectChanges()) - .toThrowError( - /It seems like the view has been created after its parent and its children have been dirty checked/); - - // subsequent change detection should run without issues - ctx.detectChanges(); - })); + it('should allow view to be created in a cd hook', () => { + const previous = ɵgetEnsureDirtyViewsAreAlwaysReachable(); + ɵsetEnsureDirtyViewsAreAlwaysReachable(true); + const ctx = createCompFixture('
{{ a }}
', TestData); + ctx.componentInstance.a = 1; + ctx.detectChanges(); + expect(ctx.nativeElement.innerText).toEqual('1'); + ɵsetEnsureDirtyViewsAreAlwaysReachable(previous); + }); it('should not throw when two arrays are structurally the same', fakeAsync(() => { const ctx = _bindSimpleValue('a', TestData);