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: Hydratation of empty lists next to components. #3630

Merged
merged 2 commits into from
Mar 12, 2024

Conversation

WorldSEnder
Copy link
Member

@WorldSEnder WorldSEnder commented Mar 12, 2024

Description

Hydrating empty lists could fail when they were placed next to suspensions and other components. The underlying issue is caused by an historical extra empty text element that was used for empty lists to make them non-empty (and give other components a dom position to hook to). For hydration though, this extra element is basically a mismatch, so shouldn't exist. It was then added to Dom, which should have triggered an error

Should not use a trapped DomSlot. Please report this as an internal bug in yew.

since it tries to insert itself before a component that hasn't received the reuse fix-up that notifies components of their hydrated siblings.

Instead, the DomSlot of a hydrating component was reassigned to the first element of its fragment. Which is, in general, wrong: Note that hydrating a component or suspension keeps some of the nodes in Dom, but crucially, extra comment nodes to delimit fragments are removed. Hence, the component's internal slot would refer to such a comment node as its "position", but the comment node was then removed when the subsequent suspense was hydrated.

#[function_component]
pub fn Bar() -> Html {
    html!{}
}
#[function_component]
pub fn App() -> Html {
    html! {
        <>
            {html! {}}
            <Bar />
        </>
    }
}

Checklist

  • I have reviewed my own code
  • I have added tests

an additional (empty) VText was inserted in the second reconciliation
pass, which tried to insert itself in an invalid position. This internal
error was masked by a "fix" of the internal slot of components, which
should have still been trapped to signal that the second fixup render
was not yet run.
github-actions[bot]
github-actions bot previously approved these changes Mar 12, 2024
Copy link

github-actions bot commented Mar 12, 2024

Benchmark - core

Yew Master

vnode           fastest       │ slowest       │ median        │ mean          │ samples │ iters
╰─ vnode_clone  2.79 ns       │ 4.015 ns      │ 3.686 ns      │ 3.468 ns      │ 100     │ 1000000000

Pull Request

vnode           fastest       │ slowest       │ median        │ mean          │ samples │ iters
╰─ vnode_clone  2.809 ns      │ 3.995 ns      │ 3.709 ns      │ 3.409 ns      │ 100     │ 1000000000

Copy link

github-actions bot commented Mar 12, 2024

Visit the preview URL for this PR (updated for commit 24d815d):

https://yew-rs-api--pr3630-hydrate-empty-list-zutzo0sl.web.app

(expires Tue, 19 Mar 2024 11:13:26 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Copy link

github-actions bot commented Mar 12, 2024

Benchmark - SSR

Yew Master

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 308.907 309.748 309.229 0.255
Hello World 10 486.169 495.646 490.179 2.939
Function Router 10 1630.702 1657.406 1643.885 7.869
Concurrent Task 10 1005.811 1006.817 1006.359 0.330
Many Providers 10 1135.718 1168.229 1150.011 11.080

Pull Request

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 308.846 311.209 309.334 0.751
Hello World 10 488.940 548.571 502.685 16.470
Function Router 10 1626.429 1651.457 1636.954 7.695
Concurrent Task 10 1005.021 1006.865 1006.292 0.629
Many Providers 10 1093.533 1149.979 1111.898 19.761

Copy link

github-actions bot commented Mar 12, 2024

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
async_clock 98.912 98.878 -0.034 -0.035%
boids 171.310 171.235 -0.074 -0.043%
communication_child_to_parent 91.610 91.558 -0.053 -0.058%
communication_grandchild_with_grandparent 103.814 103.759 -0.056 -0.054%
communication_grandparent_to_grandchild 99.186 99.123 -0.062 -0.063%
communication_parent_to_child 87.929 87.866 -0.062 -0.071%
contexts 103.887 103.815 -0.071 -0.069%
counter 84.876 84.837 -0.039 -0.046%
counter_functional 84.991 84.948 -0.043 -0.051%
dyn_create_destroy_apps 87.836 87.793 -0.043 -0.049%
file_upload 99.025 98.775 -0.250 -0.252%
function_memory_game 169.552 169.490 -0.062 -0.036%
function_router 346.077 346.023 -0.054 -0.016%
function_todomvc 159.869 159.939 +0.070 +0.044%
futures 227.215 227.276 +0.062 +0.027%
game_of_life 108.351 108.435 +0.084 +0.078%
immutable 187.587 188.354 +0.767 +0.409%
inner_html 78.724 78.674 -0.050 -0.063%
js_callback 107.506 107.197 -0.309 -0.287%
keyed_list 196.910 196.837 -0.073 -0.037%
mount_point 81.592 81.542 -0.050 -0.061%
nested_list 112.608 112.562 -0.046 -0.041%
node_refs 89.136 89.066 -0.069 -0.078%
password_strength 1707.729 1707.740 +0.011 +0.001%
portals 92.104 92.034 -0.069 -0.075%
router 315.324 315.273 -0.051 -0.016%
simple_ssr 139.439 139.067 -0.372 -0.267%
ssr_router 385.566 383.516 -2.051 -0.532%
suspense 113.910 113.836 -0.074 -0.065%
timer 87.531 87.275 -0.256 -0.292%
timer_functional 96.367 96.151 -0.216 -0.224%
todomvc 140.585 140.543 -0.042 -0.030%
two_apps 84.275 84.233 -0.042 -0.050%
web_worker_fib 133.803 133.744 -0.059 -0.044%
web_worker_prime 183.874 183.808 -0.066 -0.036%
webgl 81.371 81.321 -0.050 -0.061%

✅ None of the examples has changed their size significantly.

Copy link
Member

@ranile ranile left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for taking the time to fix it

@ranile ranile merged commit 95c29cc into yewstack:master Mar 12, 2024
23 checks passed
@WorldSEnder WorldSEnder added the A-yew Area: The main yew crate label Aug 23, 2024
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants