fix(http): wait for all XHR requests to finish before stabilizing application (#49776)

alan-agius4 · AndrewKushnir · commit 079f4bc1efc3 · 2023-04-11T13:44:30.000-07:00
Previously, since the `HttpXhrBackend` is a singleton, the macrotask was created and completed only for the initial request since it was stored as in property in the class instance. This commit replaces this logic to create a macro task for every XHR request. Closes #49730 PR Close #49776
diff --git a/goldens/public-api/common/http/index.md b/goldens/public-api/common/http/index.md
@@ -10,7 +10,6 @@ import * as i0 from '@angular/core';
 import { InjectionToken } from '@angular/core';
 import { ModuleWithProviders } from '@angular/core';
 import { Observable } from 'rxjs';
-import { OnDestroy } from '@angular/core';
 import { Provider } from '@angular/core';
 import { XhrFactory } from '@angular/common';
 
@@ -2129,12 +2128,10 @@ export interface HttpUserEvent<T> {
 }
 
 // @public
-export class HttpXhrBackend implements HttpBackend, OnDestroy {
+export class HttpXhrBackend implements HttpBackend {
     constructor(xhrFactory: XhrFactory);
     handle(req: HttpRequest<any>): Observable<HttpEvent<any>>;
     // (undocumented)
-    ngOnDestroy(): void;
-    // (undocumented)
     static ɵfac: i0.ɵɵFactoryDeclaration<HttpXhrBackend, never>;
     // (undocumented)
     static ɵprov: i0.ɵɵInjectableDeclaration<HttpXhrBackend>;
diff --git a/integration/platform-server/e2e/http-transferstate-lazy-spec.ts b/integration/platform-server/e2e/http-transferstate-lazy-spec.ts
@@ -16,12 +16,16 @@ describe('Http TransferState Lazy', function() {
     browser.driver.get(browser.baseUrl + 'http-transferstate-lazy');
 
     // Test the contents from the server.
-    const serverDiv = browser.driver.findElement(by.css('div'));
-    expect(serverDiv.getText()).toBe('API response');
+    const serverDivOne = browser.driver.findElement(by.css('div.one'));
+    expect(serverDivOne.getText()).toBe('API 1 response');
+
+    const serverDivTwo = browser.driver.findElement(by.css('div.two'));
+    expect(serverDivTwo.getText()).toBe('API 2 response');
 
     // Bootstrap the client side app and retest the contents
     browser.executeScript('doBootstrap()');
-    expect(element(by.css('div')).getText()).toBe('API response');
+    expect(element(by.css('div.one')).getText()).toBe('API 1 response');
+    expect(element(by.css('div.two')).getText()).toBe('API 2 response');
 
     // Make sure there were no client side errors.
     verifyNoBrowserErrors();
diff --git a/integration/platform-server/src/http-transferstate-lazy/transfer-state.component.ts b/integration/platform-server/src/http-transferstate-lazy/transfer-state.component.ts
@@ -10,16 +10,19 @@ import {isPlatformServer} from '@angular/common';
 import {HttpClient} from '@angular/common/http';
 import {Component, Inject, PLATFORM_ID, TransferState, makeStateKey} from '@angular/core';
 
-const httpCacheKey = makeStateKey<string>('http');
+const httpCacheKeyOne = makeStateKey<string>('http-one');
+const httpCacheKeyTwo = makeStateKey<string>('http-two');
 
 @Component({
   selector: 'transfer-state-app-http',
   template: `
-    <div>{{ response }}</div>
+    <div class="one">{{ responseOne }}</div>
+    <div class="two">{{ responseTwo }}</div>
   `,
 })
 export class TransferStateComponent {
-  response: string = '';
+  responseOne: string = '';
+  responseTwo: string = '';
 
   constructor(
     @Inject(PLATFORM_ID) private platformId: {},
@@ -30,11 +33,17 @@ export class TransferStateComponent {
   ngOnInit() {
     if (isPlatformServer(this.platformId)) {
       this.httpClient.get<any>(`http://localhost:4206/api`).subscribe((response) => {
-        this.transferState.set(httpCacheKey, response.data);
-        this.response = response.data;
+        this.transferState.set(httpCacheKeyOne, response.data);
+        this.responseOne = response.data;
+      });
+
+      this.httpClient.get<any>(`http://localhost:4206/api-2`).subscribe((response) => {
+        this.transferState.set(httpCacheKeyTwo, response.data);
+        this.responseTwo = response.data;
       });
     } else {
-      this.response = this.transferState.get(httpCacheKey, '');
+      this.responseOne = this.transferState.get(httpCacheKeyOne, '');
+      this.responseTwo = this.transferState.get(httpCacheKeyTwo, '');
     }
   }
 }
diff --git a/integration/platform-server/src/server.ts b/integration/platform-server/src/server.ts
@@ -46,7 +46,11 @@ app.get('/favicon.ico', (req, res) => {
 
 // Mock API
 app.get('/api', (req, res) => {
-  res.json({ data: 'API response'});
+  res.json({ data: 'API 1 response'});
+});
+
+app.get('/api-2', (req, res) => {
+  res.json({ data: 'API 2 response'});
 });
 
 //-----------ADD YOUR SERVER SIDE RENDERED APP HERE ----------------------
diff --git a/packages/common/http/src/xhr.ts b/packages/common/http/src/xhr.ts
@@ -7,7 +7,7 @@
  */
 
 import {XhrFactory} from '@angular/common';
-import {Injectable, OnDestroy} from '@angular/core';
+import {Injectable} from '@angular/core';
 import {Observable, Observer} from 'rxjs';
 
 import {HttpBackend} from './backend';
@@ -40,9 +40,7 @@ function getResponseUrl(xhr: any): string|null {
  * @publicApi
  */
 @Injectable()
-export class HttpXhrBackend implements HttpBackend, OnDestroy {
-  private macroTaskCanceller: VoidFunction|undefined;
-
+export class HttpXhrBackend implements HttpBackend {
   constructor(private xhrFactory: XhrFactory) {}
 
   /**
@@ -295,12 +293,14 @@ export class HttpXhrBackend implements HttpBackend, OnDestroy {
         }
       }
 
+      let macroTaskCanceller: VoidFunction|undefined;
+
       /** Tear down logic to cancel the backround macrotask. */
       const onLoadStart = () => {
-        this.macroTaskCanceller ??= createBackgroundMacroTask();
+        macroTaskCanceller ??= createBackgroundMacroTask();
       };
       const onLoadEnd = () => {
-        this.macroTaskCanceller?.();
+        macroTaskCanceller?.();
       };
 
       xhr.addEventListener('loadstart', onLoadStart);
@@ -321,7 +321,7 @@ export class HttpXhrBackend implements HttpBackend, OnDestroy {
         xhr.removeEventListener('timeout', onError);
 
         //  Cancel the background macrotask.
-        this.macroTaskCanceller?.();
+        macroTaskCanceller?.();
 
         if (req.reportProgress) {
           xhr.removeEventListener('progress', onDownProgress);
@@ -337,10 +337,6 @@ export class HttpXhrBackend implements HttpBackend, OnDestroy {
       };
     });
   }
-
-  ngOnDestroy(): void {
-    this.macroTaskCanceller?.();
-  }
 }
 
 // Cannot use `Number.MAX_VALUE` as it does not fit into a 32-bit signed integer.
