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 8000 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"
There was a problem hiding this comment.
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!
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