fix(core): When using setInput, mark view dirty in same was as markForCheck (#49711)

atscott · dylhunn · commit a4e749ffca5b · 2023-04-05T10:13:21.000-07:00
`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). PR Close #49711
diff --git a/packages/core/src/render3/component_ref.ts b/packages/core/src/render3/component_ref.ts
@@ -33,10 +33,11 @@ import {getNodeInjectable, NodeInjector} from './di';
 import {throwProviderNotFoundError} from './errors_di';
 import {registerPostOrderHooks} from './hooks';
 import {reportUnknownPropertyError} from './instructions/element_validation';
-import {addToViewTree, createLView, createTView, executeContentQueries, getOrCreateComponentTView, getOrCreateTNode, initializeDirectives, invokeDirectivesHostBindings, locateHostElement, markAsComponentHost, markDirtyIfOnPush, renderView, setInputsForProperty} from './instructions/shared';
+import {markViewDirty} from './instructions/mark_view_dirty';
+import {addToViewTree, createLView, createTView, executeContentQueries, getOrCreateComponentTView, getOrCreateTNode, initializeDirectives, invokeDirectivesHostBindings, locateHostElement, markAsComponentHost, renderView, setInputsForProperty} from './instructions/shared';
 import {ComponentDef, DirectiveDef, HostDirectiveDefs} from './interfaces/definition';
 import {PropertyAliasValue, TContainerNode, TElementContainerNode, TElementNode, TNode, TNodeType} from './interfaces/node';
-import {Renderer, RendererFactory} from './interfaces/renderer';
+import {Renderer} from './interfaces/renderer';
 import {RElement, RNode} from './interfaces/renderer_dom';
 import {CONTEXT, HEADER_OFFSET, INJECTOR, LView, LViewEnvironment, LViewFlags, TVIEW, TViewType} from './interfaces/view';
 import {MATH_ML_NAMESPACE, SVG_NAMESPACE} from './namespaces';
@@ -47,7 +48,7 @@ import {enterView, getCurrentTNode, getLView, leaveView} from './state';
 import {computeStaticStyling} from './styling/static_styling';
 import {mergeHostAttrs, setUpAttributes} from './util/attrs_utils';
 import {stringifyForError} from './util/stringify_utils';
-import {getNativeByTNode, getTNode} from './util/view_utils';
+import {getComponentLViewByIndex, getNativeByTNode, getTNode} from './util/view_utils';
 import {RootViewRef, ViewRef} from './view_ref';
 
 export class ComponentFactoryResolver extends AbstractComponentFactoryResolver {
@@ -291,7 +292,8 @@ export class ComponentRef<T> extends AbstractComponentRef<T> {
       const lView = this._rootLView;
       setInputsForProperty(lView[TVIEW], lView, dataValue, name, value);
       this.previousInputValues.set(name, value);
-      markDirtyIfOnPush(lView, this._tNode.index);
+      const childComponentLView = getComponentLViewByIndex(this._tNode.index, lView);
+      markViewDirty(childComponentLView);
     } else {
       if (ngDevMode) {
         const cmpNameForError = stringifyForError(this.componentType);
diff --git a/packages/core/test/bundling/animations/bundle.golden_symbols.json b/packages/core/test/bundling/animations/bundle.golden_symbols.json
@@ -1265,9 +1265,6 @@
   {
     "name": "markAsComponentHost"
   },
-  {
-    "name": "markDirtyIfOnPush"
-  },
   {
     "name": "markViewDirty"
   },
diff --git a/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json b/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json
@@ -1376,9 +1376,6 @@
   {
     "name": "markAsComponentHost"
   },
-  {
-    "name": "markDirtyIfOnPush"
-  },
   {
     "name": "markDuplicates"
   },
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
@@ -1334,9 +1334,6 @@
   {
     "name": "markAsComponentHost"
   },
-  {
-    "name": "markDirtyIfOnPush"
-  },
   {
     "name": "markDuplicates"
   },
diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json
@@ -1193,9 +1193,6 @@
   {
     "name": "markAsComponentHost"
   },
-  {
-    "name": "markDirtyIfOnPush"
-  },
   {
     "name": "markDuplicates"
   },
diff --git a/packages/core/test/render3/component_ref_spec.ts b/packages/core/test/render3/component_ref_spec.ts
@@ -6,12 +6,13 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
+import {ComponentRef} from '@angular/core';
 import {ComponentFactoryResolver} from '@angular/core/src/render3/component_ref';
 import {Renderer} from '@angular/core/src/render3/interfaces/renderer';
 import {RElement} from '@angular/core/src/render3/interfaces/renderer_dom';
 import {TestBed} from '@angular/core/testing';
 
-import {ChangeDetectionStrategy, Component, Injector, Input, NgModuleRef, OnChanges, Output, RendererType2, SimpleChanges, ViewEncapsulation} from '../../src/core';
+import {ChangeDetectionStrategy, Component, Injector, Input, NgModuleRef, OnChanges, Output, RendererType2, SimpleChanges, ViewChild, ViewContainerRef, ViewEncapsulation} from '../../src/core';
 import {ComponentFactory} from '../../src/linker/component_factory';
 import {RendererFactory2} from '../../src/render/api';
 import {Sanitizer} from '../../src/sanitization/sanitizer';
@@ -420,5 +421,46 @@ describe('ComponentFactory', () => {
       fixture.detectChanges();
       expect(log).toEqual(['1', '2']);
     });
+
+    it('marks parents dirty so component is not "shielded" by a non-dirty OnPush parent', () => {
+      @Component({
+        template: `{{input}}`,
+        standalone: true,
+        selector: 'dynamic',
+      })
+      class DynamicCmp {
+        @Input() input?: string;
+      }
+
+      @Component({
+        template: '<ng-template #template></ng-template>',
+        standalone: true,
+        imports: [DynamicCmp],
+        changeDetection: ChangeDetectionStrategy.OnPush,
+      })
+      class Wrapper {
+        @ViewChild('template', {read: ViewContainerRef}) template?: ViewContainerRef;
+        componentRef?: ComponentRef<DynamicCmp>;
+
+        create() {
+          this.componentRef = this.template!.createComponent(DynamicCmp);
+        }
+        setInput(value: string) {
+          this.componentRef!.setInput('input', value);
+        }
+      }
+
+      const fixture = TestBed.createComponent(Wrapper);
+      fixture.detectChanges();
+      fixture.componentInstance.create();
+
+      fixture.componentInstance.setInput('1');
+      fixture.detectChanges();
+      expect(fixture.nativeElement.innerText).toBe('1');
+
+      fixture.componentInstance.setInput('2');
+      fixture.detectChanges();
+      expect(fixture.nativeElement.innerText).toBe('2');
+    });
   });
 });

Original file line number	Diff line number	Diff line change
`@@ -1265,9 +1265,6 @@`
`1265`	`1265`	`{`
`1266`	`1266`	`"name": "markAsComponentHost"`
`1267`	`1267`	`},`
`1268`		`- {`
`1269`		`- "name": "markDirtyIfOnPush"`
`1270`		`- },`
`1271`	`1268`	`{`
`1272`	`1269`	`"name": "markViewDirty"`
`1273`	`1270`	`},`
Original file line number	Diff line number	Diff line change
`@@ -1376,9 +1376,6 @@`
`1376`	`1376`	`{`
`1377`	`1377`	`"name": "markAsComponentHost"`
`1378`	`1378`	`},`
`1379`		`- {`
`1380`		`- "name": "markDirtyIfOnPush"`
`1381`		`- },`
`1382`	`1379`	`{`
`1383`	`1380`	`"name": "markDuplicates"`
`1384`	`1381`	`},`
Original file line number	Diff line number	Diff line change
`@@ -1334,9 +1334,6 @@`
`1334`	`1334`	`{`
`1335`	`1335`	`"name": "markAsComponentHost"`
`1336`	`1336`	`},`
`1337`		`- {`
`1338`		`- "name": "markDirtyIfOnPush"`
`1339`		`- },`
`1340`	`1337`	`{`
`1341`	`1338`	`"name": "markDuplicates"`
`1342`	`1339`	`},`
Original file line number	Diff line number	Diff line change
`@@ -1193,9 +1193,6 @@`
`1193`	`1193`	`{`
`1194`	`1194`	`"name": "markAsComponentHost"`
`1195`	`1195`	`},`
`1196`		`- {`
`1197`		`- "name": "markDirtyIfOnPush"`
`1198`		`- },`
`1199`	`1196`	`{`
`1200`	`1197`	`"name": "markDuplicates"`
`1201`	`1198`	`},`