8000 fix(cdk): mark scheduler tasks as pending tasks by hoebbelsB · Pull Request #1870 · rx-angular/rx-angular · GitHub
[go: up one dir, main page]
Skip to content

fix(cdk): mark scheduler tasks as pending tasks #1870

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(cdk): mark scheduler tasks as pending tasks #1870

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f521a34
fix(cdk): mark scheduler tasks as pending tasks
hoebbelsB Apr 27, 2025
8e93170
chore(template): fix lint
hoebbelsB Apr 27, 2025
File filter

Filter by extension

8000
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 21 additions & 10 deletions libs/cdk/render-strategies/src/lib/concurrent-strategies.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { NgZone } from '@angular/core';
import { NgZone, PendingTasks } from '@angular/core';
import { coalescingManager, coalescingObj } from '@rx-angular/cdk/coalescing';
import {
cancelCallback,
Expand All @@ -17,6 +17,14 @@ import {
// set default to 60fps
forceFrameRate(60);

let pendingTasks: PendingTasks | undefined;

export function setPendingTasks(p: PendingTasks) {
if (!pendingTasks) {
pendingTasks = p;
}
}

const immediateStrategy: RxStrategyCredentials = {
Comment on lines +20 to +27
Copy link
@Platonn Platonn Apr 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks @hoebbelsB for taking care of this issue. 🙌

Do we really have to store an application-specific instance of PendingTasks class in a global context?
I'm afraid it's prone to bugs in SSR where multiple apps rendered in parallel can overwrite this single global context property, interfering with other renderings (which ideally should be independent).

More details:

  • I can see in Angular Source code, that PendingTasks is provided in the root injector, so it's scoped to a single instance of an application.
  • In SSR there can exist multiple instances of Angular applications in parallel (each being rendered for a different parallel incoming request) - all executed in the same global NodeJS context.
  • Therefore, I'm afraid in SSR parallel renderings may interfere with each other, overwriting the NodeJS global shared property let pendingTasks, which can potentially cause bugs.

Again, thank you for taking a look into this issue :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for pointing that out. It might require a bigger refactoring from our end in that case. Let me think about it

Copy link
@Platonn Platonn Apr 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I can see in the source code of RxAngular that we're now drilling down the NgZone argument deep down through the chain of functions (from RxLet, RxFor, ... directives down to scheduleOnQueue()).

I believe the usecase for the bugfix is valid.
But I can understand your concern: "is the refactoring worth it? (e.g. drilling down an instance of PendingTasks"

Copy link
@Platonn Platonn Jun 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @hoebbelsB ,
I'm not rushing for implementation, I'm aware you have plenty of responsibilities.
I could possibly go ahead and contribute a PR proposal, if only having green light from you. But I'm also not pushing for it, just asking.

So let me just ask for your architectural opinion:
Do you think it would be OK to drill down the PendingTasks object of Angular, same like we drill down the NgZone object - from the RxFor instance down through all nested function invocations down to the scheduleOnQueue() function?

I could take the human effort implement it. But I feel responsible for the long term business and architectural pros&cons of it, so I'm listing pros&cons below (feel free to add something if I missed anything):

  • pros: it unblocks us to fix the bug of RxFor (and other directives) causing the destroy of the DOM (and sometimes degrading CLS metric) despite Angular native "non-destructive-hydration" being enabled
  • cons: changing many places in the codebase just for one usecase - "pollutting" many the functions with the new (optional) param PendingTasks

Breaking changes in the TS API can be avoided thanks to:

  • some functions already being private API, so the new param can be required in TS types
  • public API functions can get the new param as an optional one

I have problem to find alternative approaches, due to my limited knowledge about the RxAngular/Template architecture. Feel free to share what alternative approaches and their pros&cons can you see. Or maybe you're fine with the approach mentioned above.

Many thanks!

name: 'immediate',
work: (cdRef) => cdRef.detectChanges(),
Expand All @@ -27,7 +35,7 @@ const immediateStrategy: RxStrategyC 8000 redentials = {
ngZone,
priority: PriorityLevel.ImmediatePriority,
scope,
})
}),
);
},
};
Expand All @@ -42,7 +50,7 @@ const userBlockingStrategy: RxStrategyCredentials = {
ngZone,
priority: PriorityLevel.UserBlockingPriority,
scope,
})
}),
);
},
};
Expand All @@ -57,7 +65,7 @@ const normalStrategy: RxStrategyCredentials = {
ngZone,
priority: PriorityLevel.NormalPriority,
scope,
})
}),
);
},
};
Expand All @@ -72,7 +80,7 @@ const lowStrategy: RxStrategyCredentials = {
ngZone,
priority: PriorityLevel.LowPriority,
scope,
})
}),
);
},
};
Expand All @@ -87,7 +95,7 @@ const idleStrategy: RxStrategyCredentials = {
ngZone,
priority: PriorityLevel.IdlePriority,
scope,
})
}),
);
},
};
Expand All @@ -99,7 +107,7 @@ function scheduleOnQueue<T>(
scope: coalescingObj;
delay?: number;
ngZone: NgZone;
}
},
): MonoTypeOperatorFunction<T> {
const scope = (options.scope as Record<string, unknown>) || {};
return (o$: Observable<T>): Observable<T> =>
Expand All @@ -108,21 +116,24 @@ function scheduleOnQueue<T>(
switchMap((v) =>
new Observable<T>((subscriber) => {
coalescingManager.add(scope);
const cleanup = pendingTasks?.add();
const task = scheduleCallback(
options.priority,
() => {
work();
coalescingManager.remove(scope);
subscriber.next(v);
cleanup?.();
},
{ delay: options.delay, ngZone: options.ngZone }
{ delay: options.delay, ngZone: options.ngZone },
);
return () => {
coalescingManager.remove(scope);
cancelCallback(task);
cleanup?.();
};
}).pipe(mapTo(v))
)
}).pipe(mapTo(v)),
),
);
}

Expand Down
28 changes: 16 additions & 12 deletions libs/cdk/render-strategies/src/lib/strategy-provider.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import {
ChangeDetectorRef,
Inject,
inject,
Injectable,
NgZone,
Optional,
PendingTasks,
} from '@angular/core';
import {
BehaviorSubject,
Expand All @@ -12,6 +14,7 @@ import {
Observable,
} from 'rxjs';
import { map, shareReplay, switchMap, takeUntil } from 'rxjs/operators';
import { setPendingTasks } from './concurrent-strategies';
import {
mergeDefaultConfig,
RX_RENDER_STRATEGIES_CONFIG,
Expand Down Expand Up @@ -108,7 +111,7 @@ export class RxStrategyProvider<T extends string = string> {
*/
set primaryStrategy(strategyName: RxStrategyNames<T>) {
this._primaryStrategy$.next(
<RxStrategyCredentials<RxStrategyNames<T>>>this.strategies[strategyName]
<RxStrategyCredentials<RxStrategyNames<T>>>this.strategies[strategyName],
);
}

Expand All @@ -131,7 +134,7 @@ export class RxStrategyProvider<T extends string = string> {
*/
readonly strategyNames$ = this.strategies$.pipe(
map((strategies) => Object.values(strategies).map((s) => s.name)),
shareReplay({ bufferSize: 1, refCount: true })
shareReplay({ bufferSize: 1, refCount: true }),
);

/**
Expand All @@ -140,8 +143,9 @@ export class RxStrategyProvider<T extends string = string> {
constructor(
@Optional()
@Inject(RX_RENDER_STRATEGIES_CONFIG)
cfg: RxRenderStrategiesConfig<T>
cfg: RxRenderStrategiesConfig<T>,
) {
setPendingTasks(inject(PendingTasks));
this._cfg = mergeDefaultConfig(cfg);
this._strategies$.next(this._cfg.customStrategies as any);
this.primaryStrategy = this.config.primaryStrategy;
Expand All @@ -165,7 +169,7 @@ export class RxStrategyProvider<T extends string = string> {
*/
scheduleWith<R>(
work: (v?: R) => void,
options?: ScheduleOnStrategyOptions
options?: ScheduleOnStrategyOptions,
): MonoTypeOperatorFunction<R> {
const strategy = this.strategies[options?.strategy || this.primaryStrategy];
const scope = options?.scope || {};
Expand All @@ -180,9 +184,9 @@ export class RxStrategyProvider<T extends string = string> {
(_v) => {
_work(_v);
},
{ scope, ngZone }
)
)
{ scope, ngZone },
),
),
);
}

Expand All @@ -202,7 +206,7 @@ export class RxStrategyProvider<T extends string = string> {
*/
schedule<R>(
work: () => R,
options?: ScheduleOnStrategyOptions
options?: ScheduleOnStrategyOptions,
): Observable<R> {
const strategy = this.strategies[options?.strategy || this.primaryStrategy];
const scope = options?.scope || {};
Expand All @@ -215,7 +219,7 @@ export class RxStrategyProvider<T extends string = string> {
() => {
returnVal = _work();
},
{ scope, ngZone }
{ scope, ngZone },
).pipe(map(() => returnVal));
}

Expand All @@ -236,7 +240,7 @@ export class RxStrategyProvider<T extends string = string> {
options?: ScheduleOnStrategyOptions & {
afterCD?: () => void;
abortCtrl?: AbortController;
}
},
): AbortController {
const strategy = this.strategies[options?.strategy || this.primaryStrategy];
const scope = options?.scope || cdRef;
Expand All @@ -254,7 +258,7 @@ export class RxStrategyProvider<T extends string = string> {
() => {
work();
},
{ scope, ngZone }
{ scope, ngZone },
)
.pipe(takeUntil(fromEvent(abC.signal, 'abort')))
.subscribe();
Expand All @@ -264,7 +268,7 @@ export class RxStrategyProvider<T extends string = string> {

function getWork<T>(
work: (args?: any) => T,
patchZone?: false | NgZone
patchZone?: false | NgZone,
): (args?: any) => T {
let _work = work;
if (patchZone) {
Expand Down
1 change: 0 additions & 1 deletion libs/template/experimental/virtual-scrolling/src/lib/virtual-scroll-element.directive.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import { unpatchedScroll } from './util';
useExisting: RxVirtualScrollElementDirective,
},
],
// eslint-disable-next-line @angular-eslint/no-host-metadata-property
host: {
class: 'rx-virtual-scroll-element',
},
Expand Down
1 change: 0 additions &am 87BF p; 1 deletion libs/template/experimental/virtual-scrolling/src/lib/virtual-scroll-viewport.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ const NG_DEV_MODE = typeof ngDevMode === 'undefined' || !!ngDevMode;
],
encapsulation: ViewEncapsulation.None,
styleUrls: ['./virtual-scroll-viewport.component.scss'],
// eslint-disable-next-line @angular-eslint/no-host-metadata-property
host: {
class: 'rx-virtual-scroll-viewport',
},
Expand Down
Loading
0