From df1dfc4c17abc6799f2e8f3f5f8604a7bf3d173a Mon Sep 17 00:00:00 2001 From: Pawel Kozlowski Date: Tue, 4 Apr 2023 15:49:38 +0200 Subject: [PATCH] fix(core): make sure that lifecycle hooks are not tracked (#49701) 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 --- packages/core/src/render3/hooks.ts | 31 ++++++++++------- .../animations/bundle.golden_symbols.json | 3 ++ .../cyclic_import/bundle.golden_symbols.json | 3 ++ .../forms_reactive/bundle.golden_symbols.json | 3 ++ .../bundle.golden_symbols.json | 3 ++ .../hello_world/bundle.golden_symbols.json | 3 ++ .../router/bundle.golden_symbols.json | 3 ++ .../bundle.golden_symbols.json | 3 ++ .../bundling/todo/bundle.golden_symbols.json | 3 ++ packages/core/test/render3/reactivity_spec.ts | 34 ++++++++++++++++++- 10 files changed, 76 insertions(+), 13 deletions(-) diff --git a/packages/core/src/render3/hooks.ts b/packages/core/src/render3/hooks.ts index 12240d42a7a..1ef9045b6e3 100644 --- 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 index fbbd31d8b7a..b7f0dcb017d 100644 --- 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 index 32fe7543bae..c00b8fe5033 100644 --- 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 index f7b27f2ca2d..f16709a59c9 100644 --- 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 index 4ba0e163e1c..797a83b7158 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 @@ -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 index 9b73403a583..a70fbce92ed 100644 --- 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 index fc4bf4c6154..84d4bcb5e06 100644 --- 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 index 712c08126ba..a788ea8eda2 100644 --- 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 index b6f7ddf240f..ddbe5ee597a 100644 --- 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 index f50fe416acc..e1188ba28ad 100644 --- 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(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: `| + `, + }) + class Cmp { + } + + const fixture = TestBed.createComponent(Cmp); + fixture.detectChanges(); + expect(fixture.nativeElement.textContent).toBe('A|B'); + }); });