From c26206963507beb3ff69bbcbf8cd003ee707afa7 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Tue, 21 Mar 2023 19:30:47 -0700 Subject: [PATCH] refactor(core): move `effect` to render3 and add `DestroyRef` integration (#49529) The `effect` implemented in the signal library is useful for testing but does not integrate with Angular. This commit moves that code to the actual framework package and integrates it with automatic cleanup via `DestroyRef`. A simpler effect implementation is used in the signal tests to test the `Watch` primitive. Further commits will update the scheduling to tie effects together with change detection. PR Close #49529 --- goldens/public-api/core/index.md | 8 +- .../src/core_reactivity_export_internal.ts | 9 ++- .../src => render3/reactivity}/effect.ts | 75 ++++++++++++------- packages/core/src/signals/index.ts | 1 - packages/core/test/render3/reactivity_spec.ts | 48 ++++++++++++ packages/core/test/signals/effect_util.ts | 32 ++++++++ .../core/test/signals/non_reactive_spec.ts | 41 ++++------ .../signals/{effect_spec.ts => watch_spec.ts} | 54 ++++++------- 8 files changed, 180 insertions(+), 88 deletions(-) rename packages/core/src/{signals/src => render3/reactivity}/effect.ts (52%) create mode 100644 packages/core/test/render3/reactivity_spec.ts create mode 100644 packages/core/test/signals/effect_util.ts rename packages/core/test/signals/{effect_spec.ts => watch_spec.ts} (62%) diff --git a/goldens/public-api/core/index.md b/goldens/public-api/core/index.md index 2952beafef9..5d59563f8e1 100644 --- a/goldens/public-api/core/index.md +++ b/goldens/public-api/core/index.md @@ -334,6 +334,12 @@ export interface CreateComputedOptions { equal?: ValueEqualityFn; } +// @public +export interface CreateEffectOptions { + injector?: Injector; + manualCleanup?: boolean; +} + // @public export function createEnvironmentInjector(providers: Array, parent: EnvironmentInjector, debugName?: string | null): EnvironmentInjector; @@ -504,7 +510,7 @@ export interface DoCheck { } // @public -export function effect(effectFn: () => void): EffectRef; +export function effect(effectFn: () => void, options?: CreateEffectOptions): EffectRef; // @public export interface EffectRef { diff --git a/packages/core/src/core_reactivity_export_internal.ts b/packages/core/src/core_reactivity_export_internal.ts index e1c7d6dc274..f80d92a2f3e 100644 --- a/packages/core/src/core_reactivity_export_internal.ts +++ b/packages/core/src/core_reactivity_export_internal.ts @@ -11,8 +11,6 @@ export { computed, CreateComputedOptions, CreateSignalOptions, - effect, - EffectRef, isSignal, Signal, signal, @@ -20,4 +18,9 @@ export { ValueEqualityFn, WritableSignal, } from './signals'; -// clang-format on +export { + CreateEffectOptions, + effect, + EffectRef, +} from './render3/reactivity/effect'; +// clang-format on diff --git a/packages/core/src/signals/src/effect.ts b/packages/core/src/render3/reactivity/effect.ts similarity index 52% rename from packages/core/src/signals/src/effect.ts rename to packages/core/src/render3/reactivity/effect.ts index e99191d53f8..598145e22d5 100644 --- a/packages/core/src/signals/src/effect.ts +++ b/packages/core/src/render3/reactivity/effect.ts @@ -6,7 +6,16 @@ * found in the LICENSE file at https://angular.io/license */ -import {Watch} from './watch'; +import {assertInInjectionContext} from '../../di/contextual'; +import {Injector} from '../../di/injector'; +import {inject} from '../../di/injector_compatibility'; +import {DestroyRef} from '../../linker/destroy_ref'; +import {Watch} from '../../signals'; + +const globalWatches = new Set(); +const queuedWatches = new Set(); + +let watchQueuePromise: {promise: Promise; resolveFn: () => void;}|null = null; /** * A global reactive effect, which can be manually destroyed. @@ -20,46 +29,60 @@ export interface EffectRef { destroy(): void; } +/** + * Options passed to the `effect` function. + * + * @developerPreview + */ +export interface CreateEffectOptions { + /** + * The `Injector` in which to create the effect. + * + * If this is not provided, the current injection context will be used instead (via `inject`). + */ + injector?: Injector; + + /** + * Whether the `effect` should require manual cleanup. + * + * If this is `false` (the default) the effect will automatically register itself to be cleaned up + * with the current `DestroyRef`. + */ + manualCleanup?: boolean; +} + /** * Create a global `Effect` for the given reactive function. * * @developerPreview */ -export function effect(effectFn: () => void): EffectRef { +export function effect(effectFn: () => void, options?: CreateEffectOptions): EffectRef { + !options?.injector && assertInInjectionContext(effect); + + const injector = options?.injector ?? inject(Injector); + const destroyRef = options?.manualCleanup !== true ? injector.get(DestroyRef) : null; + const watch = new Watch(effectFn, queueWatch); globalWatches.add(watch); // Effects start dirty. watch.notify(); + let unregisterOnDestroy: (() => void)|undefined; + + const destroy = () => { + unregisterOnDestroy?.(); + queuedWatches.delete(watch); + globalWatches.delete(watch); + }; + + unregisterOnDestroy = destroyRef?.onDestroy(destroy); + return { - destroy: () => { - queuedWatches.delete(watch); - globalWatches.delete(watch); - }, + destroy, }; } -/** - * Get a `Promise` that resolves when any scheduled effects have resolved. - */ -export function effectsDone(): Promise { - return watchQueuePromise?.promise ?? Promise.resolve(); -} - -/** - * Shut down all active effects. - */ -export function resetEffects(): void { - queuedWatches.clear(); - globalWatches.clear(); -} - -const globalWatches = new Set(); -const queuedWatches = new Set(); - -let watchQueuePromise: {promise: Promise; resolveFn: () => void;}|null = null; - function queueWatch(watch: Watch): void { if (queuedWatches.has(watch) || !globalWatches.has(watch)) { return; diff --git a/packages/core/src/signals/index.ts b/packages/core/src/signals/index.ts index 8cad986bd36..1ecf8368d4e 100644 --- a/packages/core/src/signals/index.ts +++ b/packages/core/src/signals/index.ts @@ -8,7 +8,6 @@ export {isSignal, Signal, ValueEqualityFn} from './src/api'; export {computed, CreateComputedOptions} from './src/computed'; -export {effect, EffectRef} from './src/effect'; export {setActiveConsumer} from './src/graph'; export {CreateSignalOptions, signal, WritableSignal} from './src/signal'; export {untracked} from './src/untracked'; diff --git a/packages/core/test/render3/reactivity_spec.ts b/packages/core/test/render3/reactivity_spec.ts new file mode 100644 index 00000000000..4c92f80418d --- /dev/null +++ b/packages/core/test/render3/reactivity_spec.ts @@ -0,0 +1,48 @@ +/** + * @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 + */ + +import {Component, effect, ViewChild, ViewContainerRef} from '@angular/core'; +import {bootstrapApplication} from '@angular/platform-browser'; +import {withBody} from '@angular/private/testing'; + +describe('effects', () => { + it('should run prior to change detection', withBody('', async () => { + const log: string[] = []; + @Component({ + selector: 'test-cmp', + standalone: true, + template: '', + }) + class Cmp { + constructor() { + log.push('B'); + + effect(() => { + log.push('E'); + }); + } + + ngDoCheck() { + log.push('C'); + } + } + + await bootstrapApplication(Cmp); + + expect(log).toEqual([ + // B: component bootstrapped + 'B', + // C: change detection runs -> triggers ngDoCheck + 'C', + // E: effect runs + 'E', + // C: change detection runs after effect runs + 'C', + ]); + })); +}); diff --git a/packages/core/test/signals/effect_util.ts b/packages/core/test/signals/effect_util.ts new file mode 100644 index 00000000000..bdabb4ab930 --- /dev/null +++ b/packages/core/test/signals/effect_util.ts @@ -0,0 +1,32 @@ +/** + * @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 + */ + +import {Watch} from '@angular/core/src/signals'; + +let queue = new Set(); + +/** + * A wrapper around `Watch` that emulates the `effect` API and allows for more streamlined testing. + */ +export function testingEffect(effectFn: () => void): void { + const watch = new Watch(effectFn, queue.add.bind(queue)); + + // Effects start dirty. + watch.notify(); +} + +export function flushEffects(): void { + for (const watch of queue) { + queue.delete(watch); + watch.run(); + } +} + +export function resetEffects(): void { + queue.clear(); +} diff --git a/packages/core/test/signals/non_reactive_spec.ts b/packages/core/test/signals/non_reactive_spec.ts index 7614afa60af..4bdfa2fd17f 100644 --- a/packages/core/test/signals/non_reactive_spec.ts +++ b/packages/core/test/signals/non_reactive_spec.ts @@ -7,7 +7,8 @@ */ import {computed, signal, untracked} from '@angular/core/src/signals'; -import {effect, effectsDone as flush, resetEffects} from '@angular/core/src/signals/src/effect'; + +import {flushEffects, resetEffects, testingEffect} from './effect_util'; describe('non-reactive reads', () => { afterEach(() => { @@ -43,74 +44,62 @@ describe('non-reactive reads', () => { expect(untracked(double)).toEqual(4); }); - it('should not make surrounding effect depend on the signal', async () => { + it('should not make surrounding effect depend on the signal', () => { const s = signal(1); const runLog: number[] = []; - effect(() => { + testingEffect(() => { runLog.push(untracked(s)); }); // an effect will run at least once - await flush(); + flushEffects(); expect(runLog).toEqual([1]); // subsequent signal changes should not trigger effects as signal is untracked s.set(2); - await flush(); + flushEffects(); expect(runLog).toEqual([1]); }); - it('should schedule effects on dependencies (computed) change', async () => { + it('should schedule on dependencies (computed) change', () => { const count = signal(0); const double = computed(() => count() * 2); let runLog: number[] = []; - const effectRef = effect(() => { + testingEffect(() => { runLog.push(double()); }); - await flush(); + flushEffects(); expect(runLog).toEqual([0]); count.set(1); - await flush(); - expect(runLog).toEqual([0, 2]); - - effectRef.destroy(); - count.set(2); - await flush(); + flushEffects(); expect(runLog).toEqual([0, 2]); }); - it('should non-reactively read all signals accessed inside untrack', async () => { + it('should non-reactively read all signals accessed inside untrack', () => { const first = signal('John'); const last = signal('Doe'); let runLog: string[] = []; - const effectRef = effect(() => { + const effectRef = testingEffect(() => { untracked(() => runLog.push(`${first()} ${last()}`)); }); // effects run at least once - await flush(); + flushEffects(); expect(runLog).toEqual(['John Doe']); // change one of the signals - should not update as not read reactively first.set('Patricia'); - await flush(); + flushEffects(); expect(runLog).toEqual(['John Doe']); // change one of the signals - should not update as not read reactively last.set('Garcia'); - await flush(); - expect(runLog).toEqual(['John Doe']); - - // destroy effect, should not respond to changes - effectRef.destroy(); - first.set('Robert'); - last.set('Smith'); - await flush(); + flushEffects(); expect(runLog).toEqual(['John Doe']); }); }); diff --git a/packages/core/test/signals/effect_spec.ts b/packages/core/test/signals/watch_spec.ts similarity index 62% rename from packages/core/test/signals/effect_spec.ts rename to packages/core/test/signals/watch_spec.ts index bab0527c543..cc6ad132519 100644 --- a/packages/core/test/signals/effect_spec.ts +++ b/packages/core/test/signals/watch_spec.ts @@ -7,49 +7,41 @@ */ import {computed, signal} from '@angular/core/src/signals'; -import {effect, effectsDone as flush, resetEffects} from '@angular/core/src/signals/src/effect'; -describe('effects', () => { +import {flushEffects, resetEffects, testingEffect} from './effect_util'; + +describe('watchers', () => { afterEach(() => { resetEffects(); }); - it('should create and run once effect without dependencies', async () => { + it('should create and run once, even without dependencies', () => { let runs = 0; - const effectRef = effect(() => { + testingEffect(() => { runs++; }); - await flush(); - expect(runs).toEqual(1); - - effectRef.destroy(); - await flush(); + flushEffects(); expect(runs).toEqual(1); }); - it('should schedule effects on dependencies (signal) change', async () => { + it('should schedule on dependencies (signal) change', () => { const count = signal(0); let runLog: number[] = []; - const effectRef = effect(() => { + const effectRef = testingEffect(() => { runLog.push(count()); }); - await flush(); + flushEffects(); expect(runLog).toEqual([0]); count.set(1); - await flush(); - expect(runLog).toEqual([0, 1]); - - effectRef.destroy(); - count.set(2); - await flush(); + flushEffects(); expect(runLog).toEqual([0, 1]); }); - it('should not schedule when a previous dependency changes', async () => { + it('should not schedule when a previous dependency changes', () => { const increment = (value: number) => value + 1; const countA = signal(0); const countB = signal(100); @@ -57,54 +49,54 @@ describe('effects', () => { const runLog: number[] = []; - effect(() => { + testingEffect(() => { runLog.push(useCountA() ? countA() : countB()); }); - await flush(); + flushEffects(); expect(runLog).toEqual([0]); countB.update(increment); - await flush(); + flushEffects(); // No update expected: updated the wrong signal. expect(runLog).toEqual([0]); countA.update(increment); - await flush(); + flushEffects(); expect(runLog).toEqual([0, 1]); useCountA.set(false); - await flush(); + flushEffects(); expect(runLog).toEqual([0, 1, 101]); countA.update(increment); - await flush(); + flushEffects(); // No update expected: updated the wrong signal. expect(runLog).toEqual([0, 1, 101]); }); - it('should not update dependencies of effects when dependencies don\'t change', async () => { + it('should not update dependencies when dependencies don\'t change', () => { const source = signal(0); const isEven = computed(() => source() % 2 === 0); let updateCounter = 0; - effect(() => { + testingEffect(() => { isEven(); updateCounter++; }); - await flush(); + flushEffects(); expect(updateCounter).toEqual(1); source.set(1); - await flush(); + flushEffects(); expect(updateCounter).toEqual(2); source.set(3); - await flush(); + flushEffects(); expect(updateCounter).toEqual(2); source.set(4); - await flush(); + flushEffects(); expect(updateCounter).toEqual(3); }); });