From 45a6ac09fdd2228fa4bbf5188ba8e67298754e7e Mon Sep 17 00:00:00 2001 From: Alan Agius Date: Thu, 23 Mar 2023 09:53:25 +0000 Subject: [PATCH] fix(http): force macro task creation during HTTP request (#49546) 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 --- goldens/public-api/common/http/index.md | 5 +- .../e2e/http-transferstate-lazy-spec.ts | 29 +++++ integration/platform-server/package.json | 1 + .../http-transferstate-lazy/app.component.ts | 8 ++ .../src/http-transferstate-lazy/app.server.ts | 20 +++ .../src/http-transferstate-lazy/app.ts | 30 +++++ .../src/http-transferstate-lazy/client.ts | 16 +++ .../src/http-transferstate-lazy/index.html | 11 ++ .../transfer-state.component.ts | 41 ++++++ .../transfer-state.module.ts | 20 +++ integration/platform-server/src/server.ts | 21 +++- .../platform-server/webpack.client.config.mjs | 1 + packages/common/http/src/xhr.ts | 46 ++++++- packages/platform-server/src/http.ts | 119 +++--------------- .../platform-server/test/integration_spec.ts | 17 --- 15 files changed, 260 insertions(+), 125 deletions(-) create mode 100644 integration/platform-server/e2e/http-transferstate-lazy-spec.ts create mode 100644 integration/platform-server/src/http-transferstate-lazy/app.component.ts create mode 100644 integration/platform-server/src/http-transferstate-lazy/app.server.ts create mode 100644 integration/platform-server/src/http-transferstate-lazy/app.ts create mode 100644 integration/platform-server/src/http-transferstate-lazy/client.ts create mode 100644 integration/platform-server/src/http-transferstate-lazy/index.html create mode 100644 integration/platform-server/src/http-transferstate-lazy/transfer-state.component.ts create mode 100644 integration/platform-server/src/http-transferstate-lazy/transfer-state.module.ts diff --git a/goldens/public-api/common/http/index.md b/goldens/public-api/common/http/index.md index 1cc31ac6928..c08fe630e94 100644 --- 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 { } // @public -export class HttpXhrBackend implements HttpBackend { +export class HttpXhrBackend implements HttpBackend, OnDestroy { constructor(xhrFactory: XhrFactory); handle(req: HttpRequest): Observable>; // (undocumented) + ngOnDestroy(): void; + // (undocumented) static ɵfac: i0.ɵɵFactoryDeclaration; // (undocumented) static ɵprov: i0.ɵɵInjectableDeclaration; diff --git a/integration/platform-server/e2e/http-transferstate-lazy-spec.ts b/integration/platform-server/e2e/http-transferstate-lazy-spec.ts new file mode 100644 index 00000000000..098f3326e74 --- /dev/null +++ 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 index da878efade9..4d6a374ba37 100644 --- 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 new file mode 100644 index 00000000000..40c0ecf54ec --- /dev/null +++ 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: '', +}) +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 new file mode 100644 index 00000000000..cb8c3c1ac5c --- /dev/null +++ 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 new file mode 100644 index 00000000000..9ad06e0b43e --- /dev/null +++ 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 new file mode 100644 index 00000000000..1b29048a6c1 --- /dev/null +++ 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 new file mode 100644 index 00000000000..c83f90a5d25 --- /dev/null +++ b/integration/platform-server/src/http-transferstate-lazy/index.html @@ -0,0 +1,11 @@ + + + + + Hello World + + + + + + 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 new file mode 100644 index 00000000000..a62550c84f1 --- /dev/null +++ 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('http'); + +@Component({ + selector: 'transfer-state-app-http', + template: ` +
{{ response }}
+ `, +}) +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(`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 new file mode 100644 index 00000000000..4097c21371c --- /dev/null +++ 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 index 41f84499902..ab439168369 100644 --- a/integration/platform-server/src/server.ts +++ b/integration/platform-server/src/server.ts @@ -18,6 +18,9 @@ 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, html: string) { @@ -25,7 +28,9 @@ function render(moduleType: Type, html: string) { 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 index 2e1dbf91d06..9331c8f314b 100644 --- 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 index d504edee257..63811d79255 100644 --- 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 index 4b5af4409b5..0d2968652e8 100644 --- a/packages/platform-server/src/http.ts +++ b/packages/platform-server/src/http.ts @@ -8,11 +8,11 @@ import {PlatformLocation, XhrFactory} from '@angular/common'; import {HttpBackend, HttpEvent, HttpHandler, HttpRequest, ɵHttpInterceptorHandler as HttpInterceptorHandler} from '@angular/common/http'; -import {EnvironmentInjector, inject, Injectable, Provider} from '@angular/core'; -import {Observable, Observer, Subscription} from 'rxjs'; +import {EnvironmentInjector, Inject, inject, Injectable, Provider} from '@angular/core'; +import {Observable} from 'rxjs'; import * as xhr2 from 'xhr2'; -import {INITIAL_CONFIG, PlatformConfig} from './tokens'; +import {INITIAL_CONFIG} from './tokens'; // @see https://www.w3.org/Protocols/HTTP/1.1/draft-ietf-http-v11-spec-01#URI-syntax const isAbsoluteUrl = /^[a-zA-Z\-\+.]+:\/\//; @@ -24,93 +24,20 @@ export class ServerXhr implements XhrFactory { } } -export abstract class ZoneMacroTaskWrapper { - wrap(request: S): Observable { - return new Observable((observer: Observer) => { - let task: Task = null!; - let scheduled: boolean = false; - let sub: Subscription|null = null; - let savedResult: any = null; - let savedError: any = null; +// TODO(alanagius): this logic should be re-evauted and moved into `withTransferCache` in +// `@angular/common/http` if still needed. +@Injectable() +export class ServerHttpInterceptorHandler extends HttpInterceptorHandler { + private readonly platformLocation = inject(PlatformLocation); + private readonly config = inject(INITIAL_CONFIG); - const scheduleTask = (_task: Task) => { - task = _task; - scheduled = true; - - const delegate = this.delegate(request); - sub = delegate.subscribe( - res => savedResult = res, - err => { - if (!scheduled) { - throw new Error( - 'An http observable was completed twice. This shouldn\'t happen, please file a bug.'); - } - savedError = err; - scheduled = false; - task.invoke(); - }, - () => { - if (!scheduled) { - throw new Error( - 'An http observable was completed twice. This shouldn\'t happen, please file a bug.'); - } - scheduled = false; - task.invoke(); - }); - }; - - const cancelTask = (_task: Task) => { - if (!scheduled) { - return; - } - scheduled = false; - if (sub) { - sub.unsubscribe(); - sub = null; - } - }; - - const onComplete = () => { - if (savedError !== null) { - observer.error(savedError); - } else { - observer.next(savedResult); - observer.complete(); - } - }; - - // MockBackend for Http is synchronous, which means that if scheduleTask is by - // scheduleMacroTask, the request will hit MockBackend and the response will be - // sent, causing task.invoke() to be called. - const _task = Zone.current.scheduleMacroTask( - 'ZoneMacroTaskWrapper.subscribe', onComplete, {}, () => null, cancelTask); - scheduleTask(_task); - - return () => { - if (scheduled && task) { - task.zone.cancelTask(task); - scheduled = false; - } - if (sub) { - sub.unsubscribe(); - sub = null; - } - }; - }); + constructor() { + const backend = inject(HttpBackend); + const injector = inject(EnvironmentInjector); + super(backend, injector); } - protected abstract delegate(request: S): Observable; -} - -export class ZoneClientBackend extends - ZoneMacroTaskWrapper, HttpEvent> implements HttpBackend { - constructor( - private backend: HttpBackend, private platformLocation: PlatformLocation, - private config: PlatformConfig) { - super(); - } - - handle(request: HttpRequest): Observable> { + override handle(request: HttpRequest): Observable> { const {href, protocol, hostname, port} = this.platformLocation; if (this.config.useAbsoluteUrl && !isAbsoluteUrl.test(request.url) && isAbsoluteUrl.test(href)) { @@ -118,27 +45,17 @@ export class ZoneClientBackend extends const urlPrefix = `${protocol}//${hostname}` + (port ? `:${port}` : ''); const baseUrl = new URL(baseHref, urlPrefix); const url = new URL(request.url, baseUrl); - return this.wrap(request.clone({url: url.toString()})); + return super.handle(request.clone({url: url.toString()})); } - return this.wrap(request); - } - protected override delegate(request: HttpRequest): Observable> { - return this.backend.handle(request); + return super.handle(request); } } -export function zoneWrappedInterceptorHandler( - platformLocation: PlatformLocation, config: PlatformConfig) { - return new ZoneClientBackend( - new HttpInterceptorHandler(inject(HttpBackend), inject(EnvironmentInjector)), - platformLocation, config); -} - export const SERVER_HTTP_PROVIDERS: Provider[] = [ {provide: XhrFactory, useClass: ServerXhr}, { provide: HttpHandler, - useFactory: zoneWrappedInterceptorHandler, - deps: [PlatformLocation, INITIAL_CONFIG] + useClass: ServerHttpInterceptorHandler, + deps: [PlatformLocation, INITIAL_CONFIG, HttpBackend, EnvironmentInjector] } ]; diff --git a/packages/platform-server/test/integration_spec.ts b/packages/platform-server/test/integration_spec.ts index 9bc61af92a0..df49d5086ae 100644 --- a/packages/platform-server/test/integration_spec.ts +++ b/packages/platform-server/test/integration_spec.ts @@ -1339,23 +1339,6 @@ describe('platform-server integration', () => { }); }); - it('requests are macrotasks', waitForAsync(() => { - const platform = platformDynamicServer( - [{provide: INITIAL_CONFIG, useValue: {document: ''}}]); - platform.bootstrapModule(HttpClientExampleModule).then(ref => { - const mock = ref.injector.get(HttpTestingController) as HttpTestingController; - const http = ref.injector.get(HttpClient); - ref.injector.get(NgZone).run(() => { - http.get('http://localhost/testing').subscribe((body: string) => { - expect(body).toEqual('success!'); - }); - expect(ref.injector.get(NgZone).hasPendingMacrotasks).toBeTruthy(); - mock.expectOne('http://localhost/testing').flush('success!'); - expect(ref.injector.get(NgZone).hasPendingMacrotasks).toBeFalsy(); - }); - }); - })); - it('can use HttpInterceptor that injects HttpClient', () => { const platform = platformDynamicServer([{provide: INITIAL_CONFIG, useValue: {document: ''}}]);