refactor(core): Pending effects should make the application unstable (#53835)

atscott · pkozlowski-opensource · commit e3bdab12faf7 · 2024-03-04T10:35:42.000+01:00
In zone-full applications, this is already true because effects are scheduled inside a microtask and tracked by ZoneJS. When not using zones, this should stay consistent on principle and for testability reasons. A general pattern with zoneless testing will be to update state and `await` some promise (i.e. `fixture.whenStable`, which will be linked to `ApplicationRef.isStable`). PR Close #53835
diff --git a/packages/core/src/render3/after_render_hooks.ts b/packages/core/src/render3/after_render_hooks.ts
@@ -6,11 +6,11 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {assertNotInReactiveContext} from '../core_reactivity_export_internal';
 import {assertInInjectionContext, Injector, ɵɵdefineInjectable} from '../di';
 import {inject} from '../di/injector_compatibility';
 import {ErrorHandler} from '../error_handler';
 import {DestroyRef} from '../linker/destroy_ref';
+import {assertNotInReactiveContext} from '../render3/reactivity/asserts';
 import {performanceMarkFeature} from '../util/performance';
 import {NgZone} from '../zone/ng_zone';
 
@@ -129,7 +129,7 @@ export interface InternalAfterNextRenderOptions {
   injector?: Injector;
   /**
    * When true, the hook will execute both on client and on the server.
-   * 
+   *
    * When false or undefined, the hook only executes in the browser.
    */
   runOnServer?: boolean;
diff --git a/packages/core/src/render3/reactivity/effect.ts b/packages/core/src/render3/reactivity/effect.ts
@@ -21,6 +21,7 @@ import {FLAGS, LViewFlags, EFFECTS_TO_SCHEDULE} from '../interfaces/view';
 
 import {assertNotInReactiveContext} from './asserts';
 import {performanceMarkFeature} from '../../util/performance';
+import {PendingTasks} from '../../pending_tasks';
 
 
 /**
@@ -82,18 +83,21 @@ export abstract class EffectScheduler {
  * when.
  */
 export class ZoneAwareEffectScheduler implements EffectScheduler {
-  private hasQueuedFlush = false;
   private queuedEffectCount = 0;
   private queues = new Map<Zone|null, Set<SchedulableEffect>>();
+  private readonly pendingTasks = inject(PendingTasks);
+  private taskId: number|null = null;
 
   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;
+    if (this.taskId === null) {
+      const taskId = this.taskId = this.pendingTasks.add();
+      queueMicrotask(() => {
+        this.flush();
+        this.pendingTasks.remove(taskId);
+        this.taskId = null;
+      });
     }
   }
 
diff --git a/packages/core/test/render3/reactivity_spec.ts b/packages/core/test/render3/reactivity_spec.ts
@@ -7,11 +7,12 @@
  */
 
 import {AsyncPipe} from '@angular/common';
-import {AfterViewInit, Component, computed, ContentChildren, createComponent, createEnvironmentInjector, destroyPlatform, effect, EnvironmentInjector, ErrorHandler, inject, Injectable, Injector, Input, NgZone, OnChanges, QueryList, signal, SimpleChanges, ViewChild, ViewContainerRef} from '@angular/core';
+import {AfterViewInit, ApplicationRef, Component, computed, ContentChildren, createComponent, createEnvironmentInjector, destroyPlatform, effect, EnvironmentInjector, ErrorHandler, inject, Injectable, Injector, Input, NgZone, OnChanges, QueryList, signal, SimpleChanges, ViewChild, ViewContainerRef} from '@angular/core';
 import {toObservable} from '@angular/core/rxjs-interop';
 import {TestBed} from '@angular/core/testing';
 import {bootstrapApplication} from '@angular/platform-browser';
 import {withBody} from '@angular/private/testing';
+import {filter, firstValueFrom, map} from 'rxjs';
 
 describe('effects', () => {
   beforeEach(destroyPlatform);
@@ -44,6 +45,34 @@ describe('effects', () => {
        expect(log).not.toEqual(['angular', 'angular']);
      }));
 
+  it('should contribute to application stableness when an effect is pending', async () => {
+    const someSignal = signal('initial');
+
+    @Component({
+      standalone: true,
+      template: '',
+    })
+    class App {
+      unused = effect(() => someSignal());
+    }
+
+    const appRef = TestBed.inject(ApplicationRef);
+    const componentRef =
+        createComponent(App, {environmentInjector: TestBed.inject(EnvironmentInjector)});
+    // Effect is not scheduled until change detection runs for the component
+    await expectAsync(firstValueFrom(appRef.isStable)).toBeResolvedTo(true);
+
+    componentRef.changeDetectorRef.detectChanges();
+    const stableEmits: boolean[] = [];
+    const p = firstValueFrom(appRef.isStable.pipe(
+        map(stable => {
+          stableEmits.push(stable);
+          return stableEmits;
+        }),
+        filter(emits => emits.length === 2)));
+    await expectAsync(p).toBeResolvedTo([false, true]);
+  });
+
   it('should propagate errors to the ErrorHandler', () => {
     let run = false;
 
