Block props update during hydration

[futursolo]

Description

Fixes #2664

This pull request delays any prop updates until the end of hydration and still allows next sibling refs to be synced to child components.

Checklist

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

[github-actions]

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

https://yew-rs-api--pr2665-hydration-props-z5vgarfa.web.app

_{(expires Mon, 30 May 2022 11:06:27 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

[github-actions]

Size Comparison

examples	master (KB)	pull request (KB)	diff (KB)	diff (%)
boids	172.800	172.838	+0.038	+0.022%
contexts	109.641	109.688	+0.048	+0.044%
counter	86.653	86.653	0	0.000%
counter_functional	87.306	87.306	0	0.000%
dyn_create_destroy_apps	89.771	89.744	-0.027	-0.030%
file_upload	102.620	102.620	0	0.000%
function_memory_game	167.349	167.265	-0.084	-0.050%
function_router	350.630	350.243	-0.387	-0.110%
function_todomvc	161.997	161.975	-0.022	-0.014%
futures	226.660	226.660	0	0.000%
game_of_life	107.539	107.539	0	0.000%
inner_html	83.688	83.688	0	0.000%
js_callback	112.873	112.908	+0.035	+0.031%
keyed_list	195.035	195.099	+0.063	+0.033%
mount_point	86.280	86.280	0	0.000%
nested_list	115.919	115.915	-0.004	-0.003%
node_refs	90.451	90.521	+0.070	+0.078%
password_strength	1539.628	1539.691	+0.063	+0.004%
portals	97.192	97.263	+0.070	+0.072%
router	319.672	319.340	-0.332	-0.104%
simple_ssr	494.014	494.413	+0.399	+0.081%
ssr_router	425.673	425.922	+0.249	+0.059%
suspense	110.530	110.562	+0.032	+0.029%
timer	89.362	89.362	0	0.000%
todomvc	143.054	143.054	0	0.000%
two_apps	87.288	87.288	0	0.000%
web_worker_fib	153.530	153.530	0	0.000%
webgl	87.372	87.372	0	0.000%

✅ None of the examples has changed their size significantly.

[wdcocq]

Wouldn't use_context() also be a good candidate to block propagation for? As you could write the example with use_context() and it would also fail for the same reason.
I think other hooks would be fine to not have special handling for.

[wdcocq]

I've rewritten my code away from using use_context() and it works as expected now.

[futursolo]

I've rewritten my code away from using use_context() and it works as expected now.

I think for contexts, the situation is complicated.
Suspense delays components from being created until its parent is revealed.
At that time, the context may have already changed from its initial value and the component will never get to see it.

Fixing this may require delivering context updates as scheduler runnable so initial values of all contexts will always be available to immediate parent component and hence will be visible to components during hydration.
In addition, this only works if the context does not use any interior mutability.

However, delivering context updates as scheduler runnable may cause a performance penalty on context updates.
So this may need some consideration.

I would recommend to use a prepared state (see #2650) to track the expected initial value of a context and switch to actual context value in when use_effect is called.
In the future, may be we can introduce a hook that does this automatically after #2650 is merged.

[WorldSEnder]

In addition, this only works if the context does not use any interior mutability.

Can't stop it from doing that, and apart from being clear that the only updates you get to see are the ones from prop updates to the ContextProvider, I don't think there's much to do.

In any case, I'm not quite sure how use_context is "the same". I'm under the impression that the Scope parent hierarchy of a component never changes and a component can be only be shifted in the DOM hierarchy, but never in the vDOM/bundle hierarchy, so it should be fixed and always find the same ContextProvider to subscribe to. What am I missing?

[futursolo]

In any case, I'm not quite sure how use_context is "the same".

Suppose you have a component like the following:

#[function_component]
fn Comp() -> Html {
    let num_divs = use_context::<u32>().unwrap();

    let children = 0..num_divs.map(|i| html! { <div>{i}</div> });

    html! { <>{ for children }<> }
}

The initial value of context u32 is 1 and SSR artifact also contain 1 div.

Before this component finishes hydration (e.g.: due to its parent suspending),
the context has been updated to 2 due to an update triggered by other components / callbacks.

When the component is trying to hydrate itself, it will look for 2 divs instead of 1 which would cause a hydration failure.

If interior mutability cannot be ignored, then you need to deliver a value that do not change until after the hydration with use_prepared_state.

[wdcocq]

I don't think it needs to cover interior mutability, that's straying from the intended use-case and the developer should watch out for unintended side-effects (and you can do that with properties as well..)
But use_context is described in the docs as basically a replacement for passing down properties. So if it's not going to be covered here, we should at least update the docs about this. It also hampers with the composability of components. As you would have to consider, when reusing components in SSR, whether they use contexts or not and if that has implications.

[futursolo]

I don't think it needs to cover interior mutability,

For properties, we can simply advise that interior mutability should almost never be used unless you have a very good reason and are willing to deal with any edge cases created by it.

However, for contexts, sometimes interior mutability is the only solution.

If you broadcast a Redux Store via Context, it may be subscribed by hundreds of components.
It is prohibitively expensive to actually notify all subscribers every time the store updates performance wise.
Instead of updating the context itself, Redux uses an internal mechanism to send updates to relevant subscribers (useSelector).

React 18 has a useSyncExternalStore hook to help mitigate both concurrent mode tearing and hydration initial value mismatch when dealing with such cases.

[WorldSEnder]

Before long, I might have the patch ready to change all of these sleeps to a principled stepping approach. Apart from that, I have one issue following the logic of caching props in pending_props.

[WorldSEnder]

Approving since I think it's an improvement to the current situation. However, please correct me if I'm wrong, this targets not only prop changes during hydration, but any prop changes during suspension before first render. Suppose no ssr or hydration and the following:

#[derive(PartialEq, Properties)]
struct ItemDetailProps { viewed_item: u32 }

#[function_component]
fn ItemDetails(props: ItemDetailProps) -> HtmlResult {
    let details = use_load_item_details(props.viewed_item)?;
    html! { details }
}

#[function_component]
fn ItemView() -> Html {
    let viewed_item = use_state(|| INITIAL_ITEM);
    html! {
       // ... some way to change viewed_item, maybe a text input, button, whatever
        <Suspense>
            <ItemDetails {viewed_item} />
        </Suspense>
    }
}

As far as I can tell, if the viewed_item changes while the initial item is still loading item details and suspended, it would wait for the initial load to un-suspend before triggering the re-render with the pending viewed_item prop, possibly unnecessarily delaying the actual render.

Still, better than rendering with unexpected properties during hydration.

[futursolo]

As far as I can tell, if the viewed_item changes while the initial item is still loading item details and suspended, it would wait for the initial load to un-suspend before triggering the re-render with the pending viewed_item prop, possibly unnecessarily delaying the actual render.

This is true. I have updated the code so it no longer does so.