fix(core): make sure that lifecycle hooks are not tracked (#49701)

pkozlowski-opensource · dylhunn · commit df1dfc4c17ab · 2023-04-05T10:48:22.000-07:00
Angular lifecycle hooks should never be run as part of the reactive context: we do not expect that signal reads in lifecycle hooks report to any consumers. In the current Angular some of the lifecycle hooks can be flushed early, while executting template update pass. We need to make sure that signal reads in those lifecycle hooks do not register as part of the effect that marks components for check. " PR Close #49701
diff --git a/packages/core/src/render3/hooks.ts b/packages/core/src/render3/hooks.ts
@@ -7,6 +7,7 @@
  */
 
 import {AfterContentChecked, AfterContentInit, AfterViewChecked, AfterViewInit, DoCheck, OnChanges, OnDestroy, OnInit} from '../interface/lifecycle_hooks';
+import {setActiveConsumer} from '../signals';
 import {assertDefined, assertEqual, assertNotEqual} from '../util/assert';
 
 import {assertFirstCreatePass} from './assert';
@@ -238,6 +239,22 @@ function callHooks(
   }
 }
 
+/**
+ * Executes a single lifecycle hook, making sure that:
+ * - it is called in the non-reactive context;
+ * - profiling data are registered.
+ */
+function callHookInternal(directive: any, hook: () => void) {
+  profiler(ProfilerEvent.LifecycleHookStart, directive, hook);
+  const prevConsumer = setActiveConsumer(null);
+  try {
+    hook.call(directive);
+  } finally {
+    setActiveConsumer(prevConsumer);
+    profiler(ProfilerEvent.LifecycleHookEnd, directive, hook);
+  }
+}
+
 /**
  * Execute one hook against the current `LView`.
  *
@@ -258,19 +275,9 @@ function callHook(currentView: LView, initPhase: InitPhaseState, arr: HookData,
             (currentView[PREORDER_HOOK_FLAGS] >> PreOrderHookFlags.NumberOfInitHooksCalledShift) &&
         (currentView[FLAGS] & LViewFlags.InitPhaseStateMask) === initPhase) {
       currentView[FLAGS] += LViewFlags.IndexWithinInitPhaseIncrementer;
-      profiler(ProfilerEvent.LifecycleHookStart, directive, hook);
-      try {
-        hook.call(directive);
-      } finally {
-        profiler(ProfilerEvent.LifecycleHookEnd, directive, hook);
-      }
+      callHookInternal(directive, hook);
     }
   } else {
-    profiler(ProfilerEvent.LifecycleHookStart, directive, hook);
-    try {
-      hook.call(directive);
-    } finally {
-      profiler(ProfilerEvent.LifecycleHookEnd, directive, hook);
-    }
+    callHookInternal(directive, hook);
   }
 }
diff --git a/packages/core/test/bundling/animations/bundle.golden_symbols.json b/packages/core/test/bundling/animations/bundle.golden_symbols.json
@@ -767,6 +767,9 @@
   {
     "name": "callHook"
   },
+  {
+    "name": "callHookInternal"
+  },
   {
     "name": "callHooks"
   },
diff --git a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json
@@ -572,6 +572,9 @@
   {
     "name": "callHook"
   },
+  {
+    "name": "callHookInternal"
+  },
   {
     "name": "callHooks"
   },
diff --git a/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json b/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json
@@ -770,6 +770,9 @@
   {
     "name": "callHook"
   },
+  {
+    "name": "callHookInternal"
+  },
   {
     "name": "callHooks"
   },
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
@@ -752,6 +752,9 @@
   {
     "name": "callHook"
   },
+  {
+    "name": "callHookInternal"
+  },
   {
     "name": "callHooks"
   },
diff --git a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json
@@ -440,6 +440,9 @@
   {
     "name": "callHook"
   },
+  {
+    "name": "callHookInternal"
+  },
   {
     "name": "callHooks"
   },
diff --git a/packages/core/test/bundling/router/bundle.golden_symbols.json b/packages/core/test/bundling/router/bundle.golden_symbols.json
@@ -950,6 +950,9 @@
   {
     "name": "callHook"
   },
+  {
+    "name": "callHookInternal"
+  },
   {
     "name": "callHooks"
   },
diff --git a/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json b/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json
@@ -521,6 +521,9 @@
   {
     "name": "callHook"
   },
+  {
+    "name": "callHookInternal"
+  },
   {
     "name": "callHooks"
   },
diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json
@@ -680,6 +680,9 @@
   {
     "name": "callHook"
   },
+  {
+    "name": "callHookInternal"
+  },
   {
     "name": "callHooks"
   },
diff --git a/packages/core/test/render3/reactivity_spec.ts b/packages/core/test/render3/reactivity_spec.ts
@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {AfterViewInit, Component, destroyPlatform, effect, inject, Injector, NgZone, signal} from '@angular/core';
+import {AfterViewInit, Component, destroyPlatform, effect, inject, Injector, Input, NgZone, OnChanges, signal, SimpleChanges} from '@angular/core';
 import {TestBed} from '@angular/core/testing';
 import {bootstrapApplication} from '@angular/platform-browser';
 import {withBody} from '@angular/private/testing';
@@ -216,4 +216,36 @@ describe('effects', () => {
 
        await bootstrapApplication(Cmp);
      }));
+
+  it('should allow writing to signals in ngOnChanges', () => {
+    @Component({
+      selector: 'with-input',
+      standalone: true,
+      template: '{{inSignal()}}',
+    })
+    class WithInput implements OnChanges {
+      inSignal = signal<string|undefined>(undefined);
+      @Input() in : string|undefined;
+
+      ngOnChanges(changes: SimpleChanges): void {
+        if (changes.in) {
+          this.inSignal.set(changes.in.currentValue);
+        }
+      }
+    }
+
+    @Component({
+      selector: 'test-cmp',
+      standalone: true,
+      imports: [WithInput],
+      template: `<with-input [in]="'A'" />|<with-input [in]="'B'" />
+      `,
+    })
+    class Cmp {
+    }
+
+    const fixture = TestBed.createComponent(Cmp);
+    fixture.detectChanges();
+    expect(fixture.nativeElement.textContent).toBe('A|B');
+  });
 });

Original file line number	Diff line number	Diff line change
`@@ -767,6 +767,9 @@`
`767`	`767`	`{`
`768`	`768`	`"name": "callHook"`
`769`	`769`	`},`
	`770`	`+ {`
	`771`	`+ "name": "callHookInternal"`
	`772`	`+ },`
`770`	`773`	`{`
`771`	`774`	`"name": "callHooks"`
`772`	`775`	`},`
Original file line number	Diff line number	Diff line change
`@@ -572,6 +572,9 @@`
`572`	`572`	`{`
`573`	`573`	`"name": "callHook"`
`574`	`574`	`},`
	`575`	`+ {`
	`576`	`+ "name": "callHookInternal"`
	`577`	`+ },`
`575`	`578`	`{`
`576`	`579`	`"name": "callHooks"`
`577`	`580`	`},`
Original file line number	Diff line number	Diff line change
`@@ -770,6 +770,9 @@`
`770`	`770`	`{`
`771`	`771`	`"name": "callHook"`
`772`	`772`	`},`
	`773`	`+ {`
	`774`	`+ "name": "callHookInternal"`
	`775`	`+ },`
`773`	`776`	`{`
`774`	`777`	`"name": "callHooks"`
`775`	`778`	`},`
Original file line number	Diff line number	Diff line change
`@@ -752,6 +752,9 @@`
`752`	`752`	`{`
`753`	`753`	`"name": "callHook"`
`754`	`754`	`},`
	`755`	`+ {`
	`756`	`+ "name": "callHookInternal"`
	`757`	`+ },`
`755`	`758`	`{`
`756`	`759`	`"name": "callHooks"`
`757`	`760`	`},`
Original file line number	Diff line number	Diff line change
`@@ -440,6 +440,9 @@`
`440`	`440`	`{`
`441`	`441`	`"name": "callHook"`
`442`	`442`	`},`
	`443`	`+ {`
	`444`	`+ "name": "callHookInternal"`
	`445`	`+ },`
`443`	`446`	`{`
`444`	`447`	`"name": "callHooks"`
`445`	`448`	`},`
Original file line number	Diff line number	Diff line change
`@@ -950,6 +950,9 @@`
`950`	`950`	`{`
`951`	`951`	`"name": "callHook"`
`952`	`952`	`},`
	`953`	`+ {`
	`954`	`+ "name": "callHookInternal"`
	`955`	`+ },`
`953`	`956`	`{`
`954`	`957`	`"name": "callHooks"`
`955`	`958`	`},`
Original file line number	Diff line number	Diff line change
`@@ -521,6 +521,9 @@`
`521`	`521`	`{`
`522`	`522`	`"name": "callHook"`
`523`	`523`	`},`
	`524`	`+ {`
	`525`	`+ "name": "callHookInternal"`
	`526`	`+ },`
`524`	`527`	`{`
`525`	`528`	`"name": "callHooks"`
`526`	`529`	`},`
Original file line number	Diff line number	Diff line change
`@@ -680,6 +680,9 @@`
`680`	`680`	`{`
`681`	`681`	`"name": "callHook"`
`682`	`682`	`},`
	`683`	`+ {`
	`684`	`+ "name": "callHookInternal"`
	`685`	`+ },`
`683`	`686`	`{`
`684`	`687`	`"name": "callHooks"`
`685`	`688`	`},`