-
Notifications
You must be signed in to change notification settings - Fork 842
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.
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
Update Angular to V18 #1694
Comments
I guess it could be interesting to try the new nozone with the signals one as well |
@krausest did the update fail? Or the perf results weren't good? Happy to help, just trying to understand what is going on.
nozone for sure - and those are not tied to signals (I mean, you can run zoneless without using signals). |
The update itself was fine. For angular-cf-signals I bootstrapped with provideExperimentalZonelessChangeDetection() and added ChangeDetectionStrategy.OnPush to the component. |
If it helps I set up a PR with my changes here: #1695 |
@krausest thnx for following up on this one. I will have a deeper but I don't see any "silly mistake" looking at the PR. What I do expect that the change detection scheduler we are using might not be playing nicely with the benchmark / way we measure. But yeh, this is just a suspicion for now, will profile! |
@krausest actually I think I know what is going on - at least partly.
Looking at the change in https://github.com/krausest/js-framework-benchmark/pull/1695/files#diff-ede0fde54eaba73652ad146e9d60f72e603ed149428291a7a7fc83c3d667c26aR7 it would make running the change detection at least twice. The reason for this is that we've got now zoneless scheduling change detection and manual calls to detect changes. |
I've removed double calls in a separate commit (see pkozlowski-opensource@307580b) and it significantly improves numbers but still performs worse as compared to the zone-based version. The interesting thing is that under a profiler I can see select rows performing great so, as noted above, I think that our zoneless scheduler interact in an undesirable way with the benchmarking infrastructure. I didn't get to the bottom of it yet. So I think we've got 2 issues:
Given this I would suggest the following:
WDYT? |
May I ask which is the latest modified PR? I would also like to participate and take a look |
Would be great if we moved angular to v18.
I tried to update with ng update, but results didn't look good.
Would be great if @pkozlowski-opensource or @birkskyum could take a look. Any chance?
The text was updated successfully, but these errors were encountered: