-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Delay Hydration second render until all assistive nodes have been removed #2629
Conversation
Visit the preview URL for this PR (updated for commit 0565411): https://yew-rs-api--pr2629-hydration-4-1p4413s6.web.app (expires Sun, 01 May 2022 14:36:05 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
Size Comparison
|
I still get issues with the order of a list (in a slightly different scenario as my earlier example) and a new panic:
I'm trying to figure out why exactly this happens, but if i move the |
If you turn on feature In addition, hydration expects a virtual dom layout that exactly matches the html string created during the server-side rendering process including components that do not contain any dom elements. This error can also happen if you forgot to run |
The panic was caused by dirty code from my end where two app handles were interacting with each other. Properly spawning the second app resolved the issue for me. But it still seems like it shouldn't have happened either way. But there is still an issue with list order and suspend, see: |
This has been solved as well. |
All my problems seem to be resolved with the latest changes |
Even this problem (with my hacky dirty solution) has been fixed. |
@futursolo is it necessary to revert #2597? As alluded in that PR, it's in preparation for callback refs and I don't quite understand how it's connected to the issue here. It looks as if changing the scheduler would be a sufficient solution with a smaller diffset. Sorry for not including initial hydration tests, btw, thanks for adding them. |
I initially reverted to better assess the issues around rendering order. |
It might not be part of this PR, but I just want to mention that i still get order issues, but maybe once every 20 times so not consistently. I haven't been able to reproduce it in an example yet |
This reverts commit 427b087d4db6b2e497ad618273655bd18ba9bd01, reversing changes made to 109fcfa.
This reverts commit f1e4089.
I'm currently working on getting #2551 upgraded to a sensible |
@@ -1,3 +1,5 @@ | |||
edition = "2021" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't needed (auto detected from Cargo.toml) but doesn't hurt to add
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed as some code editing integration do not use cargo fmt but rustfmt directly.
Description
Fixes #2596
Fixes #2623
Reverts #2597These issues are introduced due to a last minute change in the hydration pull request that modifies the rendering order that is used to fix NodeRefs and suspension during hydration.
#2596 is related to
node_ref
, #2623 is related tonext_sibling
.Fixes #2626
This is a similar issue that is also related to rendering order where
<Suspense />
is revealed before the inner component is able to register its suspension.Fixes
https://github.com/wdcocq/yew-issue-list/tree/suspend-order-issue-2
This is not directly related to hydration but a general issue around how next_sibling is populated during shifting.
This pull request delays the "first" render during hydration until node refs and suspensions are registered properly.
Checklist
cargo make pr-flow