Skip to content

Commit

Permalink
fix(core): TestBed should still use the microtask queue to schedule e…
Browse files Browse the repository at this point in the history
…ffects (#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 <action>,
`await` pattern described above.

PR Close #53843
  • Loading branch information
atscott committed Jan 11, 2024
1 parent 79fbc4e commit 1f8c53c
Show file tree
Hide file tree
Showing 7 changed files with 57 additions and 62 deletions.
1 change: 0 additions & 1 deletion packages/core/src/core_reactivity_export_internal.ts
Expand Up @@ -31,7 +31,6 @@ export {
EffectCleanupFn,
EffectCleanupRegisterFn,
EffectScheduler as ɵEffectScheduler,
ZoneAwareQueueingScheduler as ɵZoneAwareQueueingScheduler
} from './render3/reactivity/effect';
export {
assertNotInReactiveContext,
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/render3/interfaces/view.ts
Expand Up @@ -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';
Expand Down Expand Up @@ -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;
Expand Down
70 changes: 21 additions & 49 deletions packages/core/src/render3/reactivity/effect.ts
Expand Up @@ -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<Zone|null, Set<SchedulableEffect>>();

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());
Expand Down Expand Up @@ -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;
}
}
}

/**
Expand Down
28 changes: 27 additions & 1 deletion packages/core/test/test_bed_effect_spec.ts
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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');
});
});
8 changes: 4 additions & 4 deletions packages/core/testing/src/component_fixture.ts
Expand Up @@ -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';
Expand Down Expand Up @@ -52,7 +52,7 @@ export class ComponentFixture<T> {
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
Expand Down Expand Up @@ -134,15 +134,15 @@ export class ComponentFixture<T> {
* 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(() => {
this._tick(checkNoChanges);
});
// 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();
}

/**
Expand Down
4 changes: 2 additions & 2 deletions packages/core/testing/src/test_bed.ts
Expand Up @@ -27,6 +27,7 @@ import {
Type,
ɵconvertToBitFlags as convertToBitFlags,
ɵDeferBlockBehavior as DeferBlockBehavior,
ɵEffectScheduler as EffectScheduler,
ɵflushModuleScopingQueueAsMuchAsPossible as flushModuleScopingQueueAsMuchAsPossible,
ɵgetAsyncClassMetadataFn as getAsyncClassMetadataFn,
ɵgetUnknownElementStrictMode as getUnknownElementStrictMode,
Expand All @@ -38,7 +39,6 @@ import {
ɵsetUnknownElementStrictMode as setUnknownElementStrictMode,
ɵsetUnknownPropertyStrictMode as setUnknownPropertyStrictMode,
ɵstringify as stringify,
ɵZoneAwareQueueingScheduler as ZoneAwareQueueingScheduler,
} from '@angular/core';

/* clang-format on */
Expand Down Expand Up @@ -782,7 +782,7 @@ export class TestBedImpl implements TestBed {
* @developerPreview
*/
flushEffects(): void {
this.inject(ZoneAwareQueueingScheduler).flush();
this.inject(EffectScheduler).flush();
}
}

Expand Down
4 changes: 1 addition & 3 deletions packages/platform-browser/testing/src/browser.ts
Expand Up @@ -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() {
Expand Down Expand Up @@ -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 {
Expand Down

0 comments on commit 1f8c53c

Please sign in to comment.