fix(cdk): mark scheduler tasks as pending tasks #1870
Conversation
|
View your CI Pipeline Execution ↗ for commit 8e93170.
☁️ Nx Cloud last updated this comment at |
| let pendingTasks: PendingTasks | undefined; | ||
|
|
||
| export function setPendingTasks(p: PendingTasks) { | ||
| if (!pendingTasks) { | ||
| pendingTasks = p; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
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
PendingTasksis 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 :)
There was a problem hiding this comment.
thanks for pointing that out. It might require a bigger refactoring from our end in that case. Let me think about it
There was a problem hiding this comment.
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"
Description
fixes #1867
Not the most beautiful of all solutions, but we are lacking DI context in the scheduler.
@Platonn please have a look at the solution. I've tested it on the repository you've shared with us in your issue. For me it solved the issue