Skip to content

Commit

Permalink
fix(core): update ApplicationRef.isStable to account for rendering …
Browse files Browse the repository at this point in the history
…pending tasks

This commit updates the `ApplicationRef.isStable` API to account for
pending rendering task. This is needed as once a pending rendering task
is done, new macrotask and microtask could be created which previously caused these not
to be intercepted and thus ignored when doing SSR.
  • Loading branch information
alan-agius4 committed May 26, 2023
1 parent 6f5dabe commit 3152d06
Show file tree
Hide file tree
Showing 16 changed files with 79 additions and 76 deletions.
9 changes: 3 additions & 6 deletions packages/common/http/src/transfer_cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {APP_BOOTSTRAP_LISTENER, ApplicationRef, inject, InjectionToken, makeStateKey, Provider, StateKey, TransferState, ɵENABLED_SSR_FEATURES as ENABLED_SSR_FEATURES, ɵInitialRenderPendingTasks as InitialRenderPendingTasks} from '@angular/core';
import {APP_BOOTSTRAP_LISTENER, ApplicationRef, inject, InjectionToken, makeStateKey, Provider, StateKey, TransferState, ɵENABLED_SSR_FEATURES as ENABLED_SSR_FEATURES} from '@angular/core';
import {Observable, of} from 'rxjs';
import {first, tap} from 'rxjs/operators';

Expand Down Expand Up @@ -165,16 +165,13 @@ export function withHttpTransferCache(): Provider[] {
useFactory: () => {
const appRef = inject(ApplicationRef);
const cacheState = inject(CACHE_STATE);
const pendingTasks = inject(InitialRenderPendingTasks);

return () => {
const isStablePromise = appRef.isStable.pipe(first((isStable) => isStable)).toPromise();
isStablePromise.then(() => pendingTasks.whenAllTasksComplete).then(() => {
appRef.isStable.pipe(first((isStable) => isStable)).toPromise().then(() => {
cacheState.isCacheActive = false;
});
};
},
deps: [ApplicationRef, CACHE_STATE, InitialRenderPendingTasks]
}
}
];
}
9 changes: 7 additions & 2 deletions packages/core/src/application_ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import './util/ng_jit_mode';

import {Subscription} from 'rxjs';
import {map} from 'rxjs/operators';

import {ApplicationInitStatus} from './application_init';
import {PLATFORM_INITIALIZER} from './application_tokens';
Expand All @@ -25,6 +26,7 @@ import {ErrorHandler} from './error_handler';
import {formatRuntimeError, RuntimeError, RuntimeErrorCode} from './errors';
import {DEFAULT_LOCALE_ID} from './i18n/localization';
import {LOCALE_ID} from './i18n/tokens';
import {InitialRenderPendingTasks} from './initial_render_pending_tasks';
import {Type} from './interface/type';
import {COMPILER_OPTIONS, CompilerOptions} from './linker/compiler';
import {ComponentFactory, ComponentRef} from './linker/component_factory';
Expand All @@ -38,7 +40,7 @@ import {isStandalone} from './render3/definition';
import {assertStandaloneComponentType} from './render3/errors';
import {setLocaleId} from './render3/i18n/i18n_locale_id';
import {setJitOptions} from './render3/jit/jit_options';
import {createEnvironmentInjector, createNgModuleRefWithProviders, EnvironmentNgModuleRefAdapter, NgModuleFactory as R3NgModuleFactory} from './render3/ng_module_ref';
import {createNgModuleRefWithProviders, EnvironmentNgModuleRefAdapter, NgModuleFactory as R3NgModuleFactory} from './render3/ng_module_ref';
import {publishDefaultGlobalUtils as _publishDefaultGlobalUtils} from './render3/util/global_utils';
import {setThrowInvalidWriteToSignalError} from './signals';
import {TESTABILITY} from './testability/testability';
Expand Down Expand Up @@ -819,6 +821,7 @@ export class ApplicationRef {
/** @internal */
_views: InternalViewRef[] = [];
private readonly internalErrorHandler = inject(INTERNAL_APPLICATION_ERROR_HANDLER);
private readonly pendingTasks = inject(InitialRenderPendingTasks);

/**
* Indicates whether this instance was destroyed.
Expand All @@ -841,7 +844,9 @@ export class ApplicationRef {
/**
* Returns an Observable that indicates when the application is stable or unstable.
*/
public readonly isStable = inject(ZONE_IS_STABLE_OBSERVABLE);
public readonly isStable =
inject(ZONE_IS_STABLE_OBSERVABLE)
.pipe(map(isStable => isStable && !this.pendingTasks.hasPendingTasks));

private readonly _injector = inject(EnvironmentInjector);
/**
Expand Down
11 changes: 3 additions & 8 deletions packages/core/src/hydration/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import {Console} from '../console';
import {ENVIRONMENT_INITIALIZER, EnvironmentProviders, Injector, makeEnvironmentProviders} from '../di';
import {inject} from '../di/injector_compatibility';
import {formatRuntimeError, RuntimeErrorCode} from '../errors';
import {InitialRenderPendingTasks} from '../initial_render_pending_tasks';
import {enableLocateOrCreateContainerRefImpl} from '../linker/view_container_ref';
import {enableLocateOrCreateElementNodeImpl} from '../render3/instructions/element';
import {enableLocateOrCreateElementContainerNodeImpl} from '../render3/instructions/element_container';
Expand Down Expand Up @@ -94,9 +93,7 @@ function printHydrationStats(injector: Injector) {
/**
* Returns a Promise that is resolved when an application becomes stable.
*/
function whenStable(
appRef: ApplicationRef, pendingTasks: InitialRenderPendingTasks,
injector: Injector): Promise<unknown> {
function whenStable(appRef: ApplicationRef, injector: Injector): Promise<void> {
const isStablePromise = appRef.isStable.pipe(first((isStable: boolean) => isStable)).toPromise();
if (typeof ngDevMode !== 'undefined' && ngDevMode) {
const timeoutTime = APPLICATION_IS_STABLE_TIMEOUT;
Expand All @@ -113,8 +110,7 @@ function whenStable(
isStablePromise.finally(() => clearTimeout(timeoutId));
}

const pendingTasksPromise = pendingTasks.whenAllTasksComplete;
return Promise.allSettled([isStablePromise, pendingTasksPromise]);
return isStablePromise.then(() => {});
}

/**
Expand Down Expand Up @@ -186,10 +182,9 @@ export function withDomHydration(): EnvironmentProviders {
useFactory: () => {
if (isBrowser() && inject(IS_HYDRATION_DOM_REUSE_ENABLED)) {
const appRef = inject(ApplicationRef);
const pendingTasks = inject(InitialRenderPendingTasks);
const injector = inject(Injector);
return () => {
whenStable(appRef, pendingTasks, injector).then(() => {
whenStable(appRef, injector).then(() => {
// Wait until an app becomes stable and cleanup all views that
// were not claimed during the application bootstrap process.
// The timing is similar to when we start the serialization process
Expand Down
42 changes: 10 additions & 32 deletions packages/core/src/initial_render_pending_tasks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,7 @@
*/

import {Injectable} from './di';
import {inject} from './di/injector_compatibility';
import {OnDestroy} from './interface/lifecycle_hooks';
import {NgZone} from './zone/ng_zone';

/**
* *Internal* service that keeps track of pending tasks happening in the system
Expand All @@ -23,30 +21,11 @@ import {NgZone} from './zone/ng_zone';
@Injectable({providedIn: 'root'})
export class InitialRenderPendingTasks implements OnDestroy {
private taskId = 0;
private collection = new Set<number>();
private ngZone = inject(NgZone);
private pendingTasks = new Set<number>();
private completed = false;

private resolve!: VoidFunction;
private promise!: Promise<void>;

get whenAllTasksComplete(): Promise<void> {
if (this.collection.size === 0) {
this.complete();
}

return this.promise;
}

completed = false;

constructor() {
// Run outside of the Angular zone to avoid triggering
// extra change detection cycles.
this.ngZone.runOutsideAngular(() => {
this.promise = new Promise<void>((resolve) => {
this.resolve = resolve;
});
});
get hasPendingTasks(): boolean {
return this.pendingTasks.size > 0;
}

add(): number {
Expand All @@ -56,26 +35,25 @@ export class InitialRenderPendingTasks implements OnDestroy {
return -1;
}
const taskId = this.taskId++;
this.collection.add(taskId);
this.pendingTasks.add(taskId);
return taskId;
}

remove(taskId: number) {
remove(taskId: number): void {
if (this.completed) return;

this.collection.delete(taskId);
if (this.collection.size === 0) {
this.pendingTasks.delete(taskId);
if (this.pendingTasks.size === 0) {
this.complete();
}
}

ngOnDestroy() {
ngOnDestroy(): void {
this.complete();
this.collection.clear();
}

private complete(): void {
this.completed = true;
this.resolve();
this.pendingTasks.clear();
}
}
27 changes: 8 additions & 19 deletions packages/core/test/acceptance/initial_render_pending_tasks_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,54 +11,43 @@ import {TestBed} from '@angular/core/testing';
import {InitialRenderPendingTasks} from '../../src/initial_render_pending_tasks';

describe('InitialRenderPendingTasks', () => {
it('should resolve a promise even if there are no tasks', async () => {
const pendingTasks = TestBed.inject(InitialRenderPendingTasks);
expect(pendingTasks.completed).toBe(false);
await pendingTasks.whenAllTasksComplete;
expect(pendingTasks.completed).toBe(true);
});

it('should wait until all tasks are completed', async () => {
const pendingTasks = TestBed.inject(InitialRenderPendingTasks);
expect(pendingTasks.completed).toBe(false);
expect(pendingTasks.hasPendingTasks).toBeFalse();

const taskA = pendingTasks.add();
const taskB = pendingTasks.add();
const taskC = pendingTasks.add();
expect(pendingTasks.completed).toBe(false);
expect(pendingTasks.hasPendingTasks).toBeTrue();

pendingTasks.remove(taskA);
pendingTasks.remove(taskB);
pendingTasks.remove(taskC);
await pendingTasks.whenAllTasksComplete;
expect(pendingTasks.completed).toBe(true);
expect(pendingTasks.hasPendingTasks).toBeFalse();
});

it('should allow calls to remove the same task multiple times', async () => {
const pendingTasks = TestBed.inject(InitialRenderPendingTasks);
expect(pendingTasks.completed).toBe(false);
expect(pendingTasks.hasPendingTasks).toBeFalse();

const taskA = pendingTasks.add();

expect(pendingTasks.completed).toBe(false);
expect(pendingTasks.hasPendingTasks).toBeTrue();

pendingTasks.remove(taskA);
pendingTasks.remove(taskA);
pendingTasks.remove(taskA);

await pendingTasks.whenAllTasksComplete;
expect(pendingTasks.completed).toBe(true);
expect(pendingTasks.hasPendingTasks).toBeFalse();
});

it('should be tolerant to removal of non-existent ids', async () => {
const pendingTasks = TestBed.inject(InitialRenderPendingTasks);
expect(pendingTasks.completed).toBe(false);
expect(pendingTasks.hasPendingTasks).toBeFalse();

pendingTasks.remove(Math.random());
pendingTasks.remove(Math.random());
pendingTasks.remove(Math.random());

await pendingTasks.whenAllTasksComplete;
expect(pendingTasks.completed).toBe(true);
expect(pendingTasks.hasPendingTasks).toBeFalse();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,9 @@
{
"name": "INTERNAL_BROWSER_PLATFORM_PROVIDERS"
},
{
"name": "InitialRenderPendingTasks"
},
{
"name": "InjectFlags"
},
Expand Down Expand Up @@ -1172,6 +1175,9 @@
{
"name": "makeTimingAst"
},
{
"name": "map"
},
{
"name": "markAsComponentHost"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,9 @@
{
"name": "INTERNAL_APPLICATION_ERROR_HANDLER"
},
{
"name": "InitialRenderPendingTasks"
},
{
"name": "InjectFlags"
},
Expand Down Expand Up @@ -1238,6 +1241,9 @@
{
"name": "makeTimingAst"
},
{
"name": "map"
},
{
"name": "markAsComponentHost"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,9 @@
{
"name": "INTERNAL_APPLICATION_ERROR_HANDLER"
},
{
"name": "InitialRenderPendingTasks"
},
{
"name": "InjectFlags"
},
Expand Down Expand Up @@ -977,6 +980,9 @@
{
"name": "makeRecord"
},
{
"name": "map"
},
{
"name": "markAsComponentHost"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,9 @@
{
"name": "INTERNAL_APPLICATION_ERROR_HANDLER"
},
{
"name": "InitialRenderPendingTasks"
},
{
"name": "InjectFlags"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,9 @@
{
"name": "INTERNAL_APPLICATION_ERROR_HANDLER"
},
{
"name": "InitialRenderPendingTasks"
},
{
"name": "InjectFlags"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,9 @@
{
"name": "INTERNAL_APPLICATION_ERROR_HANDLER"
},
{
"name": "InitialRenderPendingTasks"
},
{
"name": "InjectFlags"
},
Expand Down Expand Up @@ -764,6 +767,9 @@
{
"name": "makeRecord"
},
{
"name": "map"
},
{
"name": "markViewDirty"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1052,6 +1052,9 @@
{
"name": "makeRecord"
},
{
"name": "map"
},
{
"name": "markViewDirty"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,9 @@
{
"name": "INTERNAL_BROWSER_PLATFORM_PROVIDERS"
},
{
"name": "InitialRenderPendingTasks"
},
{
"name": "InjectFlags"
},
Expand Down Expand Up @@ -857,6 +860,9 @@
{
"name": "makeRecord"
},
{
"name": "map"
},
{
"name": "markViewDirty"
},
Expand Down
Loading

0 comments on commit 3152d06

Please sign in to comment.