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

Fix issue with node refs and hydration #2597

Merged
merged 2 commits into from Apr 12, 2022

Conversation

WorldSEnder
Copy link
Member

@WorldSEnder WorldSEnder commented Apr 10, 2022

Description

When a component is contained in a component, the inner reconciles on its (actual, non-hydration) first_render, replacing the node_ref pointing to the rendered node. This left a badly linked NodeRef in the outer component's render state.

Now, keep a stable internal_ref besides the user-passed node_ref. The internal_ref never gets replaced as long as the BComp lives.

Fixes #2596


As an aside: Note that using internal_ref is also part of the machinery contained in the other component ref PR by me.

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 10, 2022
@github-actions
Copy link

github-actions bot commented Apr 10, 2022

Visit the preview URL for this PR (updated for commit 9af8012):

https://yew-rs-api--pr2597-hydration-node-refs-oq11ixgp.web.app

(expires Tue, 19 Apr 2022 00:25:13 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@github-actions
Copy link

github-actions bot commented Apr 11, 2022

Size Comparison

examples master (KB) pull request (KB) diff
boids 219.341 219.478 +0.137
contexts 136.297 136.410 +0.113
counter 111.442 111.596 +0.153
dyn_create_destroy_apps 114.877 115.029 +0.152
file_upload 127.909 128.061 +0.151
function_memory_game 214.932 215.034 +0.103
function_router 406.019 406.043 +0.024
function_todomvc 207.714 207.833 +0.119
futures 270.011 270.170 +0.159
game_of_life 139.760 139.910 +0.150
inner_html 107.222 107.374 +0.152
js_callback 114.629 114.779 +0.150
keyed_list 246.226 246.377 +0.151
mount_point 110.972 111.127 +0.155
nested_list 141.521 141.648 +0.128
node_refs 113.855 114.000 +0.145
password_strength 1618.834 1618.979 +0.145
portals 122.024 122.173 +0.148
router 371.017 371.057 +0.040
simple_ssr 573.411 573.546 +0.135
ssr_router 495.621 495.627 +0.006
suspense 135.374 135.498 +0.124
timer 113.825 113.976 +0.150
todomvc 189.500 189.650 +0.150
two_apps 111.817 111.970 +0.152
webgl 113.504 113.654 +0.150

@wdcocq
Copy link
Contributor

wdcocq commented Apr 11, 2022

This solved my issue, both in the example and my real app

When a component is contained in a component, the
inner reconciles, which used to replace the NodeRef,
which left a badly linked one in the outer Hydration render state.

Now, keep a stable internal_ref besides the user-passed node_ref.
The internal_ref never gets replaced as long as the BComp lives.
github-actions[bot]
github-actions bot previously approved these changes Apr 11, 2022
hamza1311
hamza1311 previously approved these changes Apr 11, 2022
Copy link
Member

@hamza1311 hamza1311 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment in the code stating why/how internal_ref and node_ref are different would be helpful and nice to have. You can merge it as-is and add the explainer in your other PR if you want to.

@hamza1311 hamza1311 added bug A-yew Area: The main yew crate labels Apr 12, 2022
@hamza1311 hamza1311 added this to the v0.20 milestone Apr 12, 2022
@WorldSEnder WorldSEnder merged commit 469cc34 into yewstack:master Apr 12, 2022
@WorldSEnder WorldSEnder deleted the hydration-node-refs branch April 12, 2022 22:31
futursolo added a commit to futursolo/yew that referenced this pull request Apr 20, 2022
futursolo added a commit that referenced this pull request Apr 24, 2022
…oved (#2629)

* Separate hydration and render queue.

* Revert "Fix issue with node refs and hydration (#2597)"

This reverts commit 469cc34.

* Priority Render.

* Add some tests.

* Add more tests.

* Add test result after click.

* Fix test comment.

* Fix test timing.

* Restore test.

* Once AtomicBool, now a Cell.

* Prefer use_future.

* Revealing of Suspense always happen after the component has re-rendered itself.

* Shifting should register correct next_sibling.

* Revert to HashMap.

* cargo +nightly fmt.

* Fix comment.

* Optimise Code size?

* Add comment if assertion fails.

* Revert "Merge branch 'hydration-4' into fc-prepared-state"

This reverts commit 427b087d4db6b2e497ad618273655bd18ba9bd01, reversing
changes made to 109fcfa.

* Revert "Revert "Merge branch 'hydration-4' into fc-prepared-state""

This reverts commit f1e4089.

* Redo #2957.
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
Development

Successfully merging this pull request may close these issues.

Panic using hydration when changing size of a list iterator with embedded html elements
3 participants