fix(http): force macro task creation during HTTP request (#49546)

alan-agius4 · atscott · commit 45a6ac09fdd2 · 2023-03-29T09:13:41.000-07:00
This commit adds a background macrotask when an XHR request is performed. The macrotask is started during `loadstart` and ended during `loadend` event. The macrotask is needed so that the application is not stabilized during HTTP calls. This is important for server rendering, as the application is rendering when the application is stabilized. The application is stabilized when there are no longer pending Macro and Micro tasks intercepted by Zone.js, Since an XHR request is none of these, we create a background macrotask so that Zone.js is made aware that there is something pending. Prior to this change, we patched the `HttpHandler` in `@angular/platform-server` but this is not enough, as there can be multiple `HttpHandler` in an application, example when importing `HttpClient` in a lazy loaded component/module. Which causes a new unpatched instance of `HttpHandler` to be created in the child injector which is not intercepted by Zone.js and thus the application is stabalized and rendered before the XHR request is finalized. NB: Zone.js is fundamental for SSR and currently, it's not possible to do SSR without it. Closes: #49425 PR Close #49546
diff --git a/goldens/public-api/common/http/index.md b/goldens/public-api/common/http/index.md
@@ -10,6 +10,7 @@ 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';
 
@@ -2128,10 +2129,12 @@ export interface HttpUserEvent<T> {
 }
 
 // @public
-export class HttpXhrBackend implements HttpBackend {
+export class HttpXhrBackend implements HttpBackend, OnDestroy {
     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
@@ -0,0 +1,29 @@
+/**
+ * @license
+ * Copyright Google LLC All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {browser, by, element} from 'protractor';
+
+import {verifyNoBrowserErrors} from './util';
+
+describe('Http TransferState Lazy', function() {
+  it('should transfer http state in lazy component', function() {
+    // Load the page without waiting for Angular since it is not bootstrapped automatically.
+    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');
+
+    // Bootstrap the client side app and retest the contents
+    browser.executeScript('doBootstrap()');
+    expect(element(by.css('div')).getText()).toBe('API response');
+
+    // Make sure there were no client side errors.
+    verifyNoBrowserErrors();
+  });
+});
diff --git a/integration/platform-server/package.json b/integration/platform-server/package.json
@@ -16,6 +16,7 @@
     "@angular/platform-browser": "file:../../dist/packages-dist/platform-browser",
     "@angular/platform-browser-dynamic": "file:../../dist/packages-dist/platform-browser-dynamic",
     "@angular/platform-server": "file:../../dist/packages-dist/platform-server",
+    "@angular/router": "file:../../dist/packages-dist/router",
     "express": "4.16.4",
     "rxjs": "file:../../node_modules/rxjs",
     "typescript": "file:../../node_modules/typescript",
diff --git a/integration/platform-server/src/http-transferstate-lazy/app.component.ts b/integration/platform-server/src/http-transferstate-lazy/app.component.ts
@@ -0,0 +1,8 @@
+import { Component } from '@angular/core';
+
+@Component({
+  selector: 'app-root',
+  template: '<router-outlet></router-outlet>',
+})
+export class AppComponent {
+}
diff --git a/integration/platform-server/src/http-transferstate-lazy/app.server.ts b/integration/platform-server/src/http-transferstate-lazy/app.server.ts
@@ -0,0 +1,20 @@
+/**
+ * @license
+ * Copyright Google LLC All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {NgModule} from '@angular/core';
+import {ServerModule} from '@angular/platform-server';
+
+import {HttpLazyTransferStateModule} from './app';
+import {AppComponent} from './app.component';
+
+@NgModule({
+  bootstrap: [AppComponent],
+  imports: [HttpLazyTransferStateModule, ServerModule],
+})
+export class HttpLazyTransferStateServerModule {
+}
diff --git a/integration/platform-server/src/http-transferstate-lazy/app.ts b/integration/platform-server/src/http-transferstate-lazy/app.ts
@@ -0,0 +1,30 @@
+/**
+ * @license
+ * Copyright Google LLC All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {NgModule} from '@angular/core';
+import {BrowserModule} from '@angular/platform-browser';
+import {RouterModule, Routes} from '@angular/router';
+import {AppComponent} from './app.component';
+
+const routes: Routes = [
+  {
+    path: '',
+    loadChildren: () => import('./transfer-state.module').then((m) => m.TransferStateModule),
+  },
+];
+
+@NgModule({
+  declarations: [AppComponent],
+  bootstrap: [AppComponent],
+  imports: [
+    BrowserModule,
+    RouterModule.forRoot(routes),
+  ],
+})
+export class HttpLazyTransferStateModule {
+}
diff --git a/integration/platform-server/src/http-transferstate-lazy/client.ts b/integration/platform-server/src/http-transferstate-lazy/client.ts
@@ -0,0 +1,16 @@
+/**
+ * @license
+ * Copyright Google LLC All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import 'zone.js/bundles/zone.umd';
+
+import {platformBrowser} from '@angular/platform-browser';
+import {HttpLazyTransferStateModule} from './app';
+
+window['doBootstrap'] = function() {
+  platformBrowser().bootstrapModule(HttpLazyTransferStateModule);
+};
diff --git a/integration/platform-server/src/http-transferstate-lazy/index.html b/integration/platform-server/src/http-transferstate-lazy/index.html
@@ -0,0 +1,11 @@
+<html>
+<head>
+  <base href="/http-transferstate-lazy">
+	<meta charset="UTF-8">
+	<title>Hello World</title>
+  <script src="webpack-out/httptransferstatelazy-bundle.js"></script>
+</head>
+<body>
+  <app-root></app-root>
+</body>
+</html>
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
@@ -0,0 +1,41 @@
+/**
+ * @license
+ * Copyright Google LLC All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {isPlatformServer} from '@angular/common';
+import {HttpClient} from '@angular/common/http';
+import {Component, Inject, PLATFORM_ID} from '@angular/core';
+import {TransferState, makeStateKey} from '@angular/platform-browser';
+
+const httpCacheKey = makeStateKey<string>('http');
+
+@Component({
+  selector: 'transfer-state-app-http',
+  template: `
+    <div>{{ response }}</div>
+  `,
+})
+export class TransferStateComponent {
+  response: string = '';
+
+  constructor(
+    @Inject(PLATFORM_ID) private platformId: {},
+    private readonly httpClient: HttpClient,
+    private readonly transferState: TransferState
+  ) {}
+
+  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;
+      });
+    } else {
+      this.response = this.transferState.get(httpCacheKey, '');
+    }
+  }
+}
diff --git a/integration/platform-server/src/http-transferstate-lazy/transfer-state.module.ts b/integration/platform-server/src/http-transferstate-lazy/transfer-state.module.ts
@@ -0,0 +1,20 @@
+import { CommonModule } from '@angular/common';
+import { HttpClientModule } from '@angular/common/http';
+import { NgModule } from '@angular/core';
+import { RouterModule, Routes } from '@angular/router';
+import { TransferStateComponent } from './transfer-state.component';
+
+const routes: Routes = [
+  {
+    path: '',
+    component: TransferStateComponent,
+  },
+];
+
+@NgModule({
+  imports: [RouterModule.forChild(routes), HttpClientModule, CommonModule],
+  declarations: [TransferStateComponent],
+  exports: [TransferStateComponent],
+})
+export class TransferStateModule {
+}
diff --git a/integration/platform-server/src/server.ts b/integration/platform-server/src/server.ts
@@ -18,14 +18,19 @@ const {default: helloworld} = require('raw-loader!./helloworld/index.html');
 import {TransferStateServerModule} from './transferstate/app.server';
 const {default: transferstate} = require('raw-loader!./transferstate/index.html');
 
+import {HttpLazyTransferStateServerModule} from './http-transferstate-lazy/app.server';
+const {default: httptransferstatelazy} = require('raw-loader!./http-transferstate-lazy/index.html');
+
 const app = express();
 
 function render(moduleType: Type<any>, html: string) {
   return (req, res) => {
     renderModule(moduleType, {
       document: html,
       url: req.url,
-    }).then((response) => { res.send(response); });
+    }).then((response) => {
+      res.send(response);
+    });
   };
 }
 
@@ -35,10 +40,20 @@ enableProdMode();
 app.use('/webpack-out', express.static('webpack-out'));
 
 // Keep the browser logs free of errors.
-app.get('/favicon.ico', (req, res) => { res.send(''); });
+app.get('/favicon.ico', (req, res) => {
+  res.send('');
+});
+
+// Mock API
+app.get('/api', (req, res) => {
+  res.json({ data: 'API response'});
+});
 
 //-----------ADD YOUR SERVER SIDE RENDERED APP HERE ----------------------
 app.get('/helloworld', render(HelloWorldServerModule, helloworld));
 app.get('/transferstate', render(TransferStateServerModule, transferstate));
+app.get('/http-transferstate-lazy', render(HttpLazyTransferStateServerModule, httptransferstatelazy));
 
-app.listen(4206, function() { console.log('Server listening on port 4206!'); });
+app.listen(4206, () => {
+  console.log('Server listening on port 4206!');
+});
diff --git a/integration/platform-server/webpack.client.config.mjs b/integration/platform-server/webpack.client.config.mjs
@@ -13,6 +13,7 @@ export default {
   entry: {
     helloworld: './built/src/helloworld/client.js',
     transferstate: './built/src/transferstate/client.js',
+    httptransferstatelazy: './built/src/http-transferstate-lazy/client.js',
   },
   // Allow for better debugging of this integration test.
   optimization: {minimize: false},
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} from '@angular/core';
+import {Injectable, OnDestroy} from '@angular/core';
 import {Observable, Observer} from 'rxjs';
 
 import {HttpBackend} from './backend';
@@ -40,7 +40,9 @@ function getResponseUrl(xhr: any): string|null {
  * @publicApi
  */
 @Injectable()
-export class HttpXhrBackend implements HttpBackend {
+export class HttpXhrBackend implements HttpBackend, OnDestroy {
+  private macroTaskCanceller: VoidFunction|undefined;
+
   constructor(private xhrFactory: XhrFactory) {}
 
   /**
@@ -293,18 +295,34 @@ export class HttpXhrBackend implements HttpBackend {
         }
       }
 
+      /** Tear down logic to cancel the backround macrotask. */
+      const onLoadStart = () => {
+        this.macroTaskCanceller ??= createBackgroundMacroTask();
+      };
+      const onLoadEnd = () => {
+        this.macroTaskCanceller?.();
+      };
+
+      xhr.addEventListener('loadstart', onLoadStart);
+      xhr.addEventListener('loadend', onLoadEnd);
+
       // Fire the request, and notify the event stream that it was fired.
       xhr.send(reqBody!);
       observer.next({type: HttpEventType.Sent});
-
       // This is the return from the Observable function, which is the
       // request cancellation handler.
       return () => {
         // On a cancellation, remove all registered event listeners.
+        xhr.removeEventListener('loadstart', onLoadStart);
+        xhr.removeEventListener('loadend', onLoadEnd);
         xhr.removeEventListener('error', onError);
         xhr.removeEventListener('abort', onError);
         xhr.removeEventListener('load', onLoad);
         xhr.removeEventListener('timeout', onError);
+
+        //  Cancel the background macrotask.
+        this.macroTaskCanceller?.();
+
         if (req.reportProgress) {
           xhr.removeEventListener('progress', onDownProgress);
           if (reqBody !== null && xhr.upload) {
@@ -319,4 +337,26 @@ export class HttpXhrBackend implements HttpBackend {
       };
     });
   }
+
+  ngOnDestroy(): void {
+    this.macroTaskCanceller?.();
+  }
+}
+
+// Cannot use `Number.MAX_VALUE` as it does not fit into a 32-bit signed integer.
+const MAX_INT = 2147483647;
+
+/**
+ * A method that creates a background macrotask of up to Number.MAX_VALUE.
+ *
+ * This is so that Zone.js can intercept HTTP calls, this is important for server rendering,
+ * as the application is only rendered once the application is stabilized, meaning there are pending
+ * macro and micro tasks.
+ *
+ * @returns a callback method to cancel the macrotask.
+ */
+function createBackgroundMacroTask(): VoidFunction {
+  const timeout = setTimeout(() => void 0, MAX_INT);
+
+  return () => clearTimeout(timeout);
 }
diff --git a/packages/platform-server/src/http.ts b/packages/platform-server/src/http.ts
diff --git a/packages/platform-server/test/integration_spec.ts b/packages/platform-server/test/integration_spec.ts
