From 1f8c53cd0c6c54b9d017f888567b85683ab0d348 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Mon, 8 Jan 2024 16:16:24 -0800 Subject: [PATCH] fix(core): TestBed should still use the microtask queue to schedule effects (#53843) Prior to this commit, `TestBed` would require tests call `flushEffects` or `fixture.detectChanges` in order to execute effects. In general, we want to discourage authoring tests like this because it makes the timing of change detection and effects differ from what happens in the application. Instead, developers should perform actions and `await` (or `flush`/`tick` when using `fakeAsync`) some `Promise` so that Angular can react to the changes in the same way that it does in the application. Note that this still _allows_ developers to flush effects synchronously with `flushEffects` and `detectChanges` but also enables the , `await` pattern described above. PR Close #53843 --- .../src/core_reactivity_export_internal.ts | 1 - packages/core/src/render3/interfaces/view.ts | 4 +- .../core/src/render3/reactivity/effect.ts | 70 ++++++------------- packages/core/test/test_bed_effect_spec.ts | 28 +++++++- .../core/testing/src/component_fixture.ts | 8 +-- packages/core/testing/src/test_bed.ts | 4 +- .../platform-browser/testing/src/browser.ts | 4 +- 7 files changed, 57 insertions(+), 62 deletions(-) diff --git a/packages/core/src/core_reactivity_export_internal.ts b/packages/core/src/core_reactivity_export_internal.ts index a8571c24a200e..85d179f0f30fb 100644 --- a/packages/core/src/core_reactivity_export_internal.ts +++ b/packages/core/src/core_reactivity_export_internal.ts @@ -31,7 +31,6 @@ export { EffectCleanupFn, EffectCleanupRegisterFn, EffectScheduler as ɵEffectScheduler, - ZoneAwareQueueingScheduler as ɵZoneAwareQueueingScheduler } from './render3/reactivity/effect'; export { assertNotInReactiveContext, diff --git a/packages/core/src/render3/interfaces/view.ts b/packages/core/src/render3/interfaces/view.ts index 8cf8ef5ee0303..2ed5f69e03674 100644 --- a/packages/core/src/render3/interfaces/view.ts +++ b/packages/core/src/render3/interfaces/view.ts @@ -12,7 +12,7 @@ import {DehydratedView} from '../../hydration/interfaces'; import {SchemaMetadata} from '../../metadata/schema'; import {Sanitizer} from '../../sanitization/sanitizer'; import type {ReactiveLViewConsumer} from '../reactive_lview_consumer'; -import type {FlushableEffectRunner} from '../reactivity/effect'; +import type {EffectScheduler} from '../reactivity/effect'; import type {AfterRenderEventManager} from '../after_render_hooks'; import {LContainer} from './container'; @@ -369,7 +369,7 @@ export interface LViewEnvironment { sanitizer: Sanitizer|null; /** Container for reactivity system `effect`s. */ - inlineEffectRunner: FlushableEffectRunner|null; + inlineEffectRunner: EffectScheduler|null; /** Container for after render hooks */ afterRenderEventManager: AfterRenderEventManager|null; diff --git a/packages/core/src/render3/reactivity/effect.ts b/packages/core/src/render3/reactivity/effect.ts index 6845a63350df5..4a837da9c0e60 100644 --- a/packages/core/src/render3/reactivity/effect.ts +++ b/packages/core/src/render3/reactivity/effect.ts @@ -63,33 +63,40 @@ export abstract class EffectScheduler { */ abstract scheduleEffect(e: SchedulableEffect): void; + /** + * Run any scheduled effects. + */ + abstract flush(): void; + /** @nocollapse */ static ɵprov = /** @pureOrBreakMyCode */ ɵɵdefineInjectable({ token: EffectScheduler, providedIn: 'root', - factory: () => new ZoneAwareMicrotaskScheduler(), + factory: () => new ZoneAwareEffectScheduler(), }); } /** - * Interface to an `EffectScheduler` capable of running scheduled effects synchronously. - */ -export interface FlushableEffectRunner { - /** - * Run any scheduled effects. - */ - flush(): void; -} - -/** - * An `EffectScheduler` which is capable of queueing scheduled effects per-zone, and flushing them - * as an explicit operation. + * A wrapper around `ZoneAwareQueueingScheduler` that schedules flushing via the microtask queue + * when. */ -export class ZoneAwareQueueingScheduler implements EffectScheduler, FlushableEffectRunner { +export class ZoneAwareEffectScheduler implements EffectScheduler { + private hasQueuedFlush = false; private queuedEffectCount = 0; private queues = new Map>(); scheduleEffect(handle: SchedulableEffect): void { + this.enqueue(handle); + + if (!this.hasQueuedFlush) { + queueMicrotask(() => this.flush()); + // Leave `hasQueuedFlush` as `true` so we don't queue another microtask if more effects are + // scheduled during flushing. We are guaranteed to empty the whole queue during flush. + this.hasQueuedFlush = false; + } + } + + private enqueue(handle: SchedulableEffect): void { const zone = handle.creationZone as Zone | null; if (!this.queues.has(zone)) { this.queues.set(zone, new Set()); @@ -131,41 +138,6 @@ export class ZoneAwareQueueingScheduler implements EffectScheduler, FlushableEff handle.run(); } } - - /** @nocollapse */ - static ɵprov = /** @pureOrBreakMyCode */ ɵɵdefineInjectable({ - token: ZoneAwareQueueingScheduler, - providedIn: 'root', - factory: () => new ZoneAwareQueueingScheduler(), - }); -} - -/** - * A wrapper around `ZoneAwareQueueingScheduler` that schedules flushing via the microtask queue - * when. - */ -export class ZoneAwareMicrotaskScheduler implements EffectScheduler { - private hasQueuedFlush = false; - private delegate = new ZoneAwareQueueingScheduler(); - private flushTask = () => { - // Leave `hasQueuedFlush` as `true` so we don't queue another microtask if more effects are - // scheduled during flushing. The flush of the `ZoneAwareQueueingScheduler` delegate is - // guaranteed to empty the queue. - this.delegate.flush(); - this.hasQueuedFlush = false; - - // This is a variable initialization, not a method. - // tslint:disable-next-line:semicolon - }; - - scheduleEffect(handle: SchedulableEffect): void { - this.delegate.scheduleEffect(handle); - - if (!this.hasQueuedFlush) { - queueMicrotask(this.flushTask); - this.hasQueuedFlush = true; - } - } } /** diff --git a/packages/core/test/test_bed_effect_spec.ts b/packages/core/test/test_bed_effect_spec.ts index a304fbdc64377..df58eae7eb359 100644 --- a/packages/core/test/test_bed_effect_spec.ts +++ b/packages/core/test/test_bed_effect_spec.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {Component, effect, inject, Injector} from '@angular/core'; +import {Component, effect, inject, Injector, NgZone, signal} from '@angular/core'; import {TestBed} from '@angular/core/testing'; describe('effects in TestBed', () => { @@ -85,4 +85,30 @@ describe('effects in TestBed', () => { 'Effect', ]); }); + + it('will flush effects automatically when using autoDetectChanges', async () => { + const val = signal('initial'); + let observed = ''; + @Component({ + selector: 'test-cmp', + standalone: true, + template: '', + }) + class Cmp { + constructor() { + effect(() => { + observed = val(); + }); + } + } + + const fixture = TestBed.createComponent(Cmp); + fixture.autoDetectChanges(); + + expect(observed).toBe('initial'); + val.set('new'); + expect(observed).toBe('initial'); + await fixture.whenStable(); + expect(observed).toBe('new'); + }); }); diff --git a/packages/core/testing/src/component_fixture.ts b/packages/core/testing/src/component_fixture.ts index 3907ac32c5e0d..c271f40fd6e9e 100644 --- a/packages/core/testing/src/component_fixture.ts +++ b/packages/core/testing/src/component_fixture.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {ApplicationRef, ChangeDetectorRef, ComponentRef, DebugElement, ElementRef, getDebugNode, inject, NgZone, RendererFactory2, ɵDeferBlockDetails as DeferBlockDetails, ɵgetDeferBlocks as getDeferBlocks, ɵNoopNgZone as NoopNgZone, ɵZoneAwareQueueingScheduler as ZoneAwareQueueingScheduler} from '@angular/core'; +import {ApplicationRef, ChangeDetectorRef, ComponentRef, DebugElement, ElementRef, getDebugNode, inject, NgZone, RendererFactory2, ɵDeferBlockDetails as DeferBlockDetails, ɵEffectScheduler as EffectScheduler, ɵgetDeferBlocks as getDeferBlocks, ɵNoopNgZone as NoopNgZone} from '@angular/core'; import {Subscription} from 'rxjs'; import {DeferBlockFixture} from './defer'; @@ -52,7 +52,7 @@ export class ComponentFixture { private readonly noZoneOptionIsSet = inject(ComponentFixtureNoNgZone, {optional: true}); private _ngZone: NgZone = this.noZoneOptionIsSet ? new NoopNgZone() : inject(NgZone); private _autoDetect = inject(ComponentFixtureAutoDetect, {optional: true}) ?? false; - private effectRunner = inject(ZoneAwareQueueingScheduler, {optional: true}); + private effectRunner = inject(EffectScheduler); private _subscriptions = new Subscription(); // Inject ApplicationRef to ensure NgZone stableness causes after render hooks to run // This will likely happen as a result of fixture.detectChanges because it calls ngZone.run @@ -134,7 +134,7 @@ export class ComponentFixture { * Trigger a change detection cycle for the component. */ detectChanges(checkNoChanges: boolean = true): void { - this.effectRunner?.flush(); + this.effectRunner.flush(); // Run the change detection inside the NgZone so that any async tasks as part of the change // detection are captured by the zone and can be waited for in isStable. this._ngZone.run(() => { @@ -142,7 +142,7 @@ export class ComponentFixture { }); // Run any effects that were created/dirtied during change detection. Such effects might become // dirty in response to input signals changing. - this.effectRunner?.flush(); + this.effectRunner.flush(); } /** diff --git a/packages/core/testing/src/test_bed.ts b/packages/core/testing/src/test_bed.ts index cf4fb41261c1c..04c3c3e875d16 100644 --- a/packages/core/testing/src/test_bed.ts +++ b/packages/core/testing/src/test_bed.ts @@ -27,6 +27,7 @@ import { Type, ɵconvertToBitFlags as convertToBitFlags, ɵDeferBlockBehavior as DeferBlockBehavior, + ɵEffectScheduler as EffectScheduler, ɵflushModuleScopingQueueAsMuchAsPossible as flushModuleScopingQueueAsMuchAsPossible, ɵgetAsyncClassMetadataFn as getAsyncClassMetadataFn, ɵgetUnknownElementStrictMode as getUnknownElementStrictMode, @@ -38,7 +39,6 @@ import { ɵsetUnknownElementStrictMode as setUnknownElementStrictMode, ɵsetUnknownPropertyStrictMode as setUnknownPropertyStrictMode, ɵstringify as stringify, - ɵZoneAwareQueueingScheduler as ZoneAwareQueueingScheduler, } from '@angular/core'; /* clang-format on */ @@ -782,7 +782,7 @@ export class TestBedImpl implements TestBed { * @developerPreview */ flushEffects(): void { - this.inject(ZoneAwareQueueingScheduler).flush(); + this.inject(EffectScheduler).flush(); } } diff --git a/packages/platform-browser/testing/src/browser.ts b/packages/platform-browser/testing/src/browser.ts index 81cf17d74cf5c..f086081aa9bae 100644 --- a/packages/platform-browser/testing/src/browser.ts +++ b/packages/platform-browser/testing/src/browser.ts @@ -7,7 +7,7 @@ */ import {PlatformLocation} from '@angular/common'; import {MockPlatformLocation} from '@angular/common/testing'; -import {APP_ID, createPlatformFactory, NgModule, PLATFORM_INITIALIZER, platformCore, provideZoneChangeDetection, StaticProvider, ɵEffectScheduler as EffectScheduler, ɵZoneAwareQueueingScheduler as ZoneAwareQueueingScheduler} from '@angular/core'; +import {APP_ID, createPlatformFactory, NgModule, PLATFORM_INITIALIZER, platformCore, provideZoneChangeDetection, StaticProvider} from '@angular/core'; import {BrowserModule, ɵBrowserDomAdapter as BrowserDomAdapter} from '@angular/platform-browser'; function initBrowserTests() { @@ -36,8 +36,6 @@ export const platformBrowserTesting = {provide: APP_ID, useValue: 'a'}, provideZoneChangeDetection(), {provide: PlatformLocation, useClass: MockPlatformLocation}, - {provide: ZoneAwareQueueingScheduler}, - {provide: EffectScheduler, useExisting: ZoneAwareQueueingScheduler}, ] }) export class BrowserTestingModule {