Skip to content

Navigation Menu

Sign in

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

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
Loading
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

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 31 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"

name: 'immediate',
work: (cdRef) => cdRef.detectChanges(),
Expand All @@ -27,7 +35,7 @@ const immediateStrategy: RxStrategyCredentials = {
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 28 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 1 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 & 1 deletion 1 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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.