Skip to content
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

Merged
merged 22 commits into from Apr 24, 2022

Conversation

futursolo
Copy link
Member

@futursolo futursolo commented Apr 20, 2022

Description

Fixes #2596
Fixes #2623
Reverts #2597

These 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 to next_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

  • I have run cargo make pr-flow
  • I have reviewed my own code
  • I have added tests

github-actions[bot]
github-actions bot previously approved these changes Apr 20, 2022
@github-actions
Copy link

github-actions bot commented Apr 20, 2022

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 🌎

github-actions[bot]
github-actions bot previously approved these changes Apr 20, 2022
@github-actions
Copy link

github-actions bot commented Apr 20, 2022

Size Comparison

examples master (KB) pull request (KB) diff
boids 173.700 174.101 +0.400
contexts 110.697 111.078 +0.381
counter 87.387 87.359 -0.027
counter_functional 87.942 87.963 +0.021
dyn_create_destroy_apps 90.438 90.451 +0.014
file_upload 103.315 103.337 +0.021
function_memory_game 167.818 168.204 +0.386
function_router 353.774 354.312 +0.537
function_todomvc 162.742 163.123 +0.381
futures 227.144 227.165 +0.021
game_of_life 108.423 108.444 +0.021
inner_html 83.881 83.908 +0.027
js_callback 113.453 113.610 +0.157
keyed_list 196.321 196.761 +0.439
mount_point 86.905 86.926 +0.021
nested_list 116.410 116.807 +0.396
node_refs 90.274 90.712 +0.438
password_strength 1540.549 1540.988 +0.439
portals 97.477 97.910 +0.434
router 319.629 319.867 +0.238
simple_ssr 500.123 500.687 +0.563
ssr_router 429.379 430.135 +0.756
suspense 111.175 111.326 +0.151
timer 90.070 90.088 +0.018
todomvc 144.103 144.124 +0.021
two_apps 87.969 87.989 +0.021
webgl 87.561 87.582 +0.021

examples/js_callback/Cargo.toml Show resolved Hide resolved
packages/yew/src/html/component/lifecycle.rs Outdated Show resolved Hide resolved
packages/yew/tests/hydration.rs Show resolved Hide resolved
packages/yew/tests/hydration.rs Outdated Show resolved Hide resolved
@hamza1311 hamza1311 added bug A-yew Area: The main yew crate labels Apr 20, 2022
@hamza1311 hamza1311 added this to the v0.20 milestone Apr 20, 2022
@wdcocq
Copy link
Contributor

wdcocq commented Apr 21, 2022

I still get issues with the order of a list (in a slightly different scenario as my earlier example) and a new panic:

panicked at 'failed to remove child element: JsValue(NotFoundError: Node.removeChild: The node to be removed is not a child of this node

I'm trying to figure out why exactly this happens, but if i move the <Suspense> to a lower level it does work. So it seems very sensitive to the correct placement.

github-actions[bot]
github-actions bot previously approved these changes Apr 21, 2022
github-actions[bot]
github-actions bot previously approved these changes Apr 21, 2022
@futursolo
Copy link
Member Author

futursolo commented Apr 21, 2022

I'm trying to figure out why exactly this happens, but if i move the to a lower level it does work. So it seems very sensitive to the correct placement.

If you turn on feature trace_hydration, you will be able to see which component has failed during the hydration process.

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 trunk build and / or restart the server after changing the source code.

@wdcocq
Copy link
Contributor

wdcocq commented Apr 22, 2022

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:
https://github.com/wdcocq/yew-issue-list/tree/suspend-order-issue-2

github-actions[bot]
github-actions bot previously approved these changes Apr 22, 2022
@futursolo
Copy link
Member Author

But there is still an issue with list order and suspend, see:
https://github.com/wdcocq/yew-issue-list/tree/suspend-order-issue-2

This has been solved as well.
This is not related to hydration but in general an issue with shifting where the next_sibling is not populated properly.

github-actions[bot]
github-actions bot previously approved these changes Apr 22, 2022
github-actions[bot]
github-actions bot previously approved these changes Apr 22, 2022
@wdcocq
Copy link
Contributor

wdcocq commented Apr 22, 2022

All my problems seem to be resolved with the latest changes

@wdcocq
Copy link
Contributor

wdcocq commented Apr 22, 2022

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.

Even this problem (with my hacky dirty solution) has been fixed.

@WorldSEnder
Copy link
Member

WorldSEnder commented Apr 22, 2022

@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.

@futursolo
Copy link
Member Author

@futursolo is it necessary to revert #2597?

I initially reverted to better assess the issues around rendering order.
However, after fixing the issue, I think a good portion of the changes are not needed if landed alongside with removal of public node ref for components (mainly around renaming the node ref). So I decided to keep it reverted.

github-actions[bot]
github-actions bot previously approved these changes Apr 24, 2022
@wdcocq
Copy link
Contributor

wdcocq commented Apr 24, 2022

This has been solved as well.
This is not related to hydration but in general an issue with shifting where the next_sibling is not populated properly.

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.
github-actions[bot]
github-actions bot previously approved these changes Apr 24, 2022
@WorldSEnder
Copy link
Member

WorldSEnder commented Apr 24, 2022

However, after fixing the issue, I think a good portion of the changes are not needed if landed alongside with removal of public node ref for components (mainly around renaming the node ref). So I decided to keep it reverted.

I'm currently working on getting #2551 upgraded to a sensible HtmlRef and it will cause unnecessary merge conflicts (more or less reverting the revert, but slightly adapting it to HtmlRef) if it's changed here :/

@futursolo
Copy link
Member Author

futursolo commented Apr 24, 2022

if it's changed here :/

Although this is not going to help conflict resolution in any way, I have reapplied #2597.

Let's assess these changes in #2551 instead.

@@ -1,3 +1,5 @@
edition = "2021"
Copy link
Member

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

Copy link
Member Author

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.

@futursolo futursolo merged commit 2576372 into yewstack:master Apr 24, 2022
@futursolo futursolo deleted the hydration-4 branch December 15, 2022 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew Area: The main yew crate bug
Projects
None yet
4 participants