fix(http): Only contribute to ApplicationRef.isStable indicator in …

…the `HttpBackend` This commit updates the approach to how the `http` package contributes to application stability by moving the contribution point from an interceptor wrapper to the backend. `HttpClient` uses the `PendingTasks` service to contribute to application stability. This was added in v16 to support SSR without relying on an infinite `setTimeout` with ZoneJS like it did pre-v16. Prior to version 16, this was also only done on the server and did not affect clients or unit tests. (angular@28c68f7) === Additional background information === Prior to angular#54949, `PendingTasks` contribute to `ApplicationRef.isStable` but did not contribute to the stability of `ComponentFixture`. This divergence in stability behavior was not intended. By aligning the two behaviors again, this includes all pending tasks in the stability of fixtures. This is likely to be a pretty large breaking change test with `HttpClient`. Tests appear to quite often use `await fixture.whenStable` when there are unfinished requests that have not been mocked or flushed. This change prevents request in `HttpClient` from contributing to stability through the `PendingTasks` in tests by not including the stability contribution in the mock backend. In this scenario, requests need to be flushed manually for them to resolve, which is problematic for existing test suites which do not flush them before `await fixture.whenStable`. BREAKING CHANGE: `HttpClient` interceptors are no longer contribute directly to `ApplicationRef.isStable` without ZoneJS. `ApplicationRef.isStable` is used for SSR to determine when to serialize the application. If there is async work in interceptors that is not captured by ZoneJS and needs to be included in the SSR serialization, these should be updated to keep the zone unstable by running a timeout inside the zone and clearing it when the async interceptor work is finished.
atscott · Apr 8, 2024 · 8901e19 · 8901e19
1 parent 840c375
commit 8901e19
Show file tree

Hide file tree

Showing 9 changed files with 259 additions and 29 deletions.
diff --git a/packages/common/http/src/fetch.ts b/packages/common/http/src/fetch.ts
@@ -6,8 +6,9 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {inject, Injectable, NgZone} from '@angular/core';
+import {inject, Injectable, NgZone, ɵPendingTasks as PendingTasks} from '@angular/core';
 import {Observable, Observer} from 'rxjs';
+import {finalize} from 'rxjs/operators';
 
 import {HttpBackend} from './backend';
 import {HttpHeaders} from './headers';
@@ -56,15 +57,21 @@ export class FetchBackend implements HttpBackend {
   private readonly fetchImpl =
     inject(FetchFactory, {optional: true})?.fetch ?? fetch.bind(globalThis);
   private readonly ngZone = inject(NgZone);
+  private readonly pendingTasks = inject(PendingTasks);
 
   handle(request: HttpRequest<any>): Observable<HttpEvent<any>> {
-    return new Observable((observer) => {
+    const pendingTask = this.pendingTasks.add();
+    return new Observable<HttpEvent<any>>((observer) => {
       const aborter = new AbortController();
       this.doRequest(request, aborter.signal, observer).then(noop, (error) =>
         observer.error(new HttpErrorResponse({error})),
       );
       return () => aborter.abort();
-    });
+    }).pipe(
+      finalize(() => {
+        this.pendingTasks.remove(pendingTask);
+      }),
+    );
   }
 
   private async doRequest(

diff --git a/packages/common/http/src/interceptor.ts b/packages/common/http/src/interceptor.ts
@@ -16,7 +16,6 @@ import {
   runInInjectionContext,
   ɵConsole as Console,
   ɵformatRuntimeError as formatRuntimeError,
-  ɵPendingTasks as PendingTasks,
 } from '@angular/core';
 import {Observable} from 'rxjs';
 import {finalize} from 'rxjs/operators';
@@ -242,9 +241,7 @@ export function legacyInterceptorFnFactory(): HttpInterceptorFn {
       );
     }
 
-    const pendingTasks = inject(PendingTasks);
-    const taskId = pendingTasks.add();
-    return chain(req, handler).pipe(finalize(() => pendingTasks.remove(taskId)));
+    return chain(req, handler);
   };
 }
 
@@ -258,7 +255,6 @@ export function resetFetchBackendWarningFlag() {
 @Injectable()
 export class HttpInterceptorHandler extends HttpHandler {
   private chain: ChainedInterceptorFn<unknown> | null = null;
-  private readonly pendingTasks = inject(PendingTasks);
 
   constructor(
     private backend: HttpBackend,
@@ -316,9 +312,8 @@ export class HttpInterceptorHandler extends HttpHandler {
       );
     }
 
-    const taskId = this.pendingTasks.add();
     return this.chain(initialRequest, (downstreamRequest) =>
       this.backend.handle(downstreamRequest),
-    ).pipe(finalize(() => this.pendingTasks.remove(taskId)));
+    );
   }
 }
diff --git a/packages/common/http/src/jsonp.ts b/packages/common/http/src/jsonp.ts
@@ -13,8 +13,9 @@ import {
   inject,
   Injectable,
   runInInjectionContext,
+  ɵPendingTasks as PendingTasks,
 } from '@angular/core';
-import {Observable, Observer} from 'rxjs';
+import {finalize, Observable, Observer} from 'rxjs';
 
 import {HttpBackend, HttpHandler} from './backend';
 import {HttpHandlerFn} from './interceptor';
@@ -92,6 +93,7 @@ export class JsonpClientBackend implements HttpBackend {
    * A resolved promise that can be used to schedule microtasks in the event handlers.
    */
   private readonly resolvedPromise = Promise.resolve();
+  private readonly pendingTasks = inject(PendingTasks);
 
   constructor(
     private callbackMap: JsonpCallbackContext,
@@ -127,6 +129,7 @@ export class JsonpClientBackend implements HttpBackend {
     }
 
     // Everything else happens inside the Observable boundary.
+    const pendingTask = this.pendingTasks.add();
     return new Observable<HttpEvent<any>>((observer: Observer<HttpEvent<any>>) => {
       // The first step to make a request is to generate the callback name, and replace the
       // callback placeholder in the URL with the name. Care has to be taken here to ensure
@@ -251,7 +254,11 @@ export class JsonpClientBackend implements HttpBackend {
         // And finally, clean up the page.
         cleanup();
       };
-    });
+    }).pipe(
+      finalize(() => {
+        this.pendingTasks.remove(pendingTask);
+      }),
+    );
   }
 
   private removeListeners(script: HTMLScriptElement): void {

diff --git a/packages/common/http/src/xhr.ts b/packages/common/http/src/xhr.ts
@@ -7,9 +7,14 @@
  */
 
 import {XhrFactory} from '@angular/common';
-import {Injectable, ɵRuntimeError as RuntimeError} from '@angular/core';
+import {
+  Injectable,
+  ɵRuntimeError as RuntimeError,
+  ɵPendingTasks as PendingTasks,
+  inject,
+} from '@angular/core';
 import {from, Observable, Observer, of} from 'rxjs';
-import {switchMap} from 'rxjs/operators';
+import {finalize, switchMap} from 'rxjs/operators';
 
 import {HttpBackend} from './backend';
 import {RuntimeErrorCode} from './errors';
@@ -52,6 +57,7 @@ function getResponseUrl(xhr: any): string | null {
  */
 @Injectable()
 export class HttpXhrBackend implements HttpBackend {
+  private readonly pendingTasks = inject(PendingTasks);
   constructor(private xhrFactory: XhrFactory) {}
 
   /**
@@ -80,6 +86,7 @@ export class HttpXhrBackend implements HttpBackend {
 
     return source.pipe(
       switchMap(() => {
+        const pendingTask = this.pendingTasks.add();
         // Everything happens on Observable subscription.
         return new Observable((observer: Observer<HttpEvent<any>>) => {
           // Start by setting up the XHR object with request method, URL, and withCredentials
@@ -347,7 +354,11 @@ export class HttpXhrBackend implements HttpBackend {
               xhr.abort();
             }
           };
-        });
+        }).pipe(
+          finalize(() => {
+            this.pendingTasks.remove(pendingTask);
+          }),
+        );
       }),
     );
   }

diff --git a/packages/common/http/test/fetch_spec.ts b/packages/common/http/test/fetch_spec.ts
@@ -8,8 +8,8 @@
 
 import {HttpEvent, HttpEventType, HttpRequest, HttpResponse} from '@angular/common/http';
 import {TestBed} from '@angular/core/testing';
-import {Observable, of, Subject} from 'rxjs';
-import {catchError, retry, scan, skip, take, toArray} from 'rxjs/operators';
+import {firstValueFrom, Observable, of, Subject} from 'rxjs';
+import {catchError, filter, retry, scan, skip, take, toArray} from 'rxjs/operators';
 
 import {
   HttpDownloadProgressEvent,
@@ -19,6 +19,7 @@ import {
   HttpStatusCode,
 } from '../public_api';
 import {FetchBackend, FetchFactory} from '../src/fetch';
+import {ApplicationRef} from '@angular/core';
 
 function trackEvents(obs: Observable<any>): Promise<any[]> {
   return obs
@@ -68,6 +69,72 @@ describe('FetchBackend', async () => {
     backend = TestBed.inject(FetchBackend);
   });
 
+  describe('stability', () => {
+    const post = TEST_POST.clone({
+      setHeaders: {
+        'Test': 'Test header',
+      },
+    });
+    function whenStable() {
+      return firstValueFrom(TestBed.inject(ApplicationRef).isStable.pipe(filter((s) => !!s)));
+    }
+
+    it('should contribute to stability until backend request completes', async () => {
+      let stable = false;
+      TestBed.inject(ApplicationRef).isStable.subscribe((v) => {
+        stable = v;
+      });
+
+      expect(stable).toBe(true);
+      backend.handle(TEST_POST).subscribe();
+      expect(stable).toBe(false);
+      fetchMock.mockFlush(HttpStatusCode.Ok, 'OK', 'some response');
+      await expectAsync(whenStable()).toBeResolved();
+    });
+
+    it('should contribute to stability on progress events', async () => {
+      let stable = false;
+      TestBed.inject(ApplicationRef).isStable.subscribe((v) => {
+        stable = v;
+      });
+
+      expect(stable).toBe(true);
+      backend.handle(TEST_POST).subscribe();
+      expect(stable).toBe(false);
+      fetchMock.mockProgressEvent(4);
+      await new Promise<void>((resolve) => setTimeout(resolve));
+      expect(stable).toBe(false);
+      fetchMock.mockFlush(HttpStatusCode.Ok, 'OK', 'some response');
+      await expectAsync(whenStable()).toBeResolved();
+    });
+
+    it('should contribute to stability until backend request errors', async () => {
+      let stable = false;
+      TestBed.inject(ApplicationRef).isStable.subscribe((v) => {
+        stable = v;
+      });
+
+      expect(stable).toBe(true);
+      backend.handle(TEST_POST).subscribe({error: () => {}});
+      expect(stable).toBe(false);
+      fetchMock.mockErrorEvent(HttpStatusCode.Ok);
+      await expectAsync(whenStable()).toBeResolved();
+    });
+
+    it('should contribute to stability until backend request aborts', async () => {
+      let stable = false;
+      TestBed.inject(ApplicationRef).isStable.subscribe((v) => {
+        stable = v;
+      });
+
+      expect(stable).toBe(true);
+      backend.handle(TEST_POST).subscribe({error: () => {}});
+      expect(stable).toBe(false);
+      fetchMock.mockAbortEvent();
+      await expectAsync(whenStable()).toBeResolved();
+    });
+  });
+
   it('emits status immediately', () => {
     let event!: HttpEvent<any>;
     // subscribe is sync

diff --git a/packages/common/http/test/jsonp_spec.ts b/packages/common/http/test/jsonp_spec.ts
@@ -16,9 +16,12 @@ import {
 } from '@angular/common/http/src/jsonp';
 import {HttpRequest} from '@angular/common/http/src/request';
 import {HttpErrorResponse, HttpEventType} from '@angular/common/http/src/response';
-import {toArray} from 'rxjs/operators';
+import {filter, toArray} from 'rxjs/operators';
 
 import {MockDocument} from './jsonp_mock';
+import {TestBed} from '@angular/core/testing';
+import {firstValueFrom} from 'rxjs';
+import {ApplicationRef} from '@angular/core';
 
 describe('JsonpClientBackend', () => {
   const SAMPLE_REQ = new HttpRequest<never>('JSONP', '/test');
@@ -36,7 +39,40 @@ describe('JsonpClientBackend', () => {
   beforeEach(() => {
     home = {};
     document = new MockDocument();
-    backend = new JsonpClientBackend(home, document);
+    backend = TestBed.runInInjectionContext(() => new JsonpClientBackend(home, document));
+  });
+
+  describe('stability', () => {
+    function whenStable() {
+      return firstValueFrom(TestBed.inject(ApplicationRef).isStable.pipe(filter((s) => !!s)));
+    }
+
+    it('should contribute to stability until backend request completes', async () => {
+      let stable = false;
+      TestBed.inject(ApplicationRef).isStable.subscribe((v) => {
+        stable = v;
+      });
+
+      expect(stable).toBe(true);
+      backend.handle(SAMPLE_REQ).subscribe();
+      expect(stable).toBe(false);
+      runOnlyCallback(home, {data: 'This is a test'});
+      document.mockLoad();
+      await expectAsync(whenStable()).toBeResolved();
+    });
+
+    it('should contribute to stability until backend request errors', async () => {
+      let stable = false;
+      TestBed.inject(ApplicationRef).isStable.subscribe((v) => {
+        stable = v;
+      });
+
+      expect(stable).toBe(true);
+      backend.handle(SAMPLE_REQ).subscribe({error: () => {}});
+      expect(stable).toBe(false);
+      document.mockLoad();
+      await expectAsync(whenStable()).toBeResolved();
+    });
   });
 
   it('handles a basic request', (done) => {

diff --git a/packages/common/http/test/provider_spec.ts b/packages/common/http/test/provider_spec.ts
@@ -26,6 +26,7 @@ import {
   provideHttpClientTesting,
 } from '@angular/common/http/testing';
 import {
+  ApplicationRef,
   createEnvironmentInjector,
   EnvironmentInjector,
   inject,
@@ -34,7 +35,7 @@ import {
   Provider,
 } from '@angular/core';
 import {TestBed} from '@angular/core/testing';
-import {EMPTY, Observable} from 'rxjs';
+import {EMPTY, Observable, from} from 'rxjs';
 
 import {HttpInterceptorFn, resetFetchBackendWarningFlag} from '../src/interceptor';
 import {
@@ -76,6 +77,23 @@ describe('provideHttpClient', () => {
     TestBed.inject(HttpTestingController).expectOne('/test').flush('');
   });
 
+  it('should not contribute to stability', () => {
+    TestBed.configureTestingModule({
+      providers: [provideHttpClient(), provideHttpClientTesting()],
+    });
+
+    let stable = false;
+    TestBed.inject(ApplicationRef).isStable.subscribe((v) => {
+      stable = v;
+    });
+
+    expect(stable).toBe(true);
+    TestBed.inject(HttpClient).get('/test', {responseType: 'text'}).subscribe();
+    expect(stable).toBe(true);
+    TestBed.inject(HttpTestingController).expectOne('/test').flush('');
+    expect(stable).toBe(true);
+  });
+
   it('should not use legacy interceptors by default', () => {
     TestBed.configureTestingModule({
       providers: [