From a317874f9362354fbd80c023d378e90b4aca3928 Mon Sep 17 00:00:00 2001 From: Greg Johnston Date: Mon, 11 Sep 2023 21:01:35 -0400 Subject: [PATCH] change: run effects after a tick (#1680) --- docs/book/src/ssr/24_hydration_bugs.md | 37 --- examples/todomvc/src/lib.rs | 8 +- leptos_dom/src/components/dyn_child.rs | 322 +++++++++++++------------ leptos_dom/src/components/each.rs | 156 ++++++------ leptos_dom/src/html.rs | 2 +- leptos_dom/src/node_ref.rs | 4 +- leptos_reactive/src/effect.rs | 49 +++- 7 files changed, 294 insertions(+), 284 deletions(-) diff --git a/docs/book/src/ssr/24_hydration_bugs.md b/docs/book/src/ssr/24_hydration_bugs.md index 667969168..90cbfb500 100644 --- a/docs/book/src/ssr/24_hydration_bugs.md +++ b/docs/book/src/ssr/24_hydration_bugs.md @@ -87,43 +87,6 @@ The WASM version of your app, running in the browser, expects to find three item It’s pretty rare that you do this intentionally, but it could happen from somehow running different logic on the server and in the browser. If you’re seeing warnings like this and you don’t think it’s your fault, it’s much more likely that it’s a bug with `` or something. Feel free to go ahead and open an [issue](https://github.com/leptos-rs/leptos/issues) or [discussion](https://github.com/leptos-rs/leptos/discussions) on GitHub for help. -### Mutating the DOM during rendering - -This is a slightly more common way to create a client/server mismatch: updating a signal _during rendering_ in a way that mutates the view. - -```rust -#[component] -pub fn App() -> impl IntoView { - let (loaded, set_loaded) = create_signal(false); - - // create_effect only runs on the client - create_effect(move |_| { - // do something like reading from localStorage - set_loaded(true); - }); - - move || { - if loaded() { - view! {

"Hello, world!"

}.into_any() - } else { - view! {
"Loading..."
}.into_any() - } - } -} -``` - -This one gives us the scary panic - -``` -panicked at 'assertion failed: `(left == right)` - left: `"DIV"`, - right: `"P"`: SSR and CSR elements have the same hydration key but different node kinds. -``` - -And a handy link to this page! - -The problem here is that `create_effect` runs **immediately** and **synchronously**, but only in the browser. As a result, on the server, `loaded` is false, and a `
` is rendered. But on the browser, by the time the view is being rendered, `loaded` has already been set to `true`, and the browser is expecting to find a `

`. - #### Solution You can simply tell the effect to wait a tick before updating the signal, by using something like `request_animation_frame`, which will set a short timeout and then update the signal before the next frame. diff --git a/examples/todomvc/src/lib.rs b/examples/todomvc/src/lib.rs index 37b5d389c..4b70b522a 100644 --- a/examples/todomvc/src/lib.rs +++ b/examples/todomvc/src/lib.rs @@ -210,13 +210,7 @@ pub fn TodoMVC() -> impl IntoView { // focus the main input on load create_effect(move |_| { if let Some(input) = input_ref.get() { - // We use request_animation_frame here because the NodeRef - // is filled when the element is created, but before it's mounted - // to the DOM. Calling .focus() before it's mounted does nothing. - // So inside, we wait a tick for the browser to mount it, then .focus() - request_animation_frame(move || { - let _ = input.focus(); - }); + let _ = input.focus(); } }); diff --git a/leptos_dom/src/components/dyn_child.rs b/leptos_dom/src/components/dyn_child.rs index 6b11f09ae..5eb6ac85f 100644 --- a/leptos_dom/src/components/dyn_child.rs +++ b/leptos_dom/src/components/dyn_child.rs @@ -7,7 +7,7 @@ use std::{cell::RefCell, fmt, ops::Deref, rc::Rc}; cfg_if! { if #[cfg(all(target_arch = "wasm32", feature = "web"))] { use crate::{mount_child, prepare_to_move, unmount_child, MountKind, Mountable, Text}; - use leptos_reactive::create_effect; + use leptos_reactive::create_render_effect; use wasm_bindgen::JsCast; } } @@ -181,194 +181,204 @@ where let span = tracing::Span::current(); #[cfg(all(target_arch = "wasm32", feature = "web"))] - create_effect(move |prev_run: Option>| { - #[cfg(debug_assertions)] - let _guard = span.enter(); + create_render_effect( + move |prev_run: Option>| { + #[cfg(debug_assertions)] + let _guard = span.enter(); - let new_child = child_fn().into_view(); + let new_child = child_fn().into_view(); - let mut child_borrow = child.borrow_mut(); + let mut child_borrow = child.borrow_mut(); - // Is this at least the second time we are loading a child? - if let Some(prev_t) = prev_run { - let child = child_borrow.take().unwrap(); + // Is this at least the second time we are loading a child? + if let Some(prev_t) = prev_run { + let child = child_borrow.take().unwrap(); - // We need to know if our child wasn't moved elsewhere. - // If it was, `DynChild` no longer "owns" that child, and - // is therefore no longer sound to unmount it from the DOM - // or to reuse it in the case of a text node + // We need to know if our child wasn't moved elsewhere. + // If it was, `DynChild` no longer "owns" that child, and + // is therefore no longer sound to unmount it from the DOM + // or to reuse it in the case of a text node - // TODO check does this still detect moves correctly? - let was_child_moved = prev_t.is_none() - && child - .get_closing_node() - .next_non_view_marker_sibling() - .as_ref() - != Some(&closing); + // TODO check does this still detect moves correctly? + let was_child_moved = prev_t.is_none() + && child + .get_closing_node() + .next_non_view_marker_sibling() + .as_ref() + != Some(&closing); - // If the previous child was a text node, we would like to - // make use of it again if our current child is also a text - // node - let ret = if let Some(prev_t) = prev_t { - // Here, our child is also a text node + // If the previous child was a text node, we would like to + // make use of it again if our current child is also a text + // node + let ret = if let Some(prev_t) = prev_t { + // Here, our child is also a text node - // nb: the match/ownership gymnastics here - // are so that, if we can reuse the text node, - // we can take ownership of new_t so we don't clone - // the contents, which in O(n) on the length of the text - if matches!(new_child, View::Text(_)) { - if !was_child_moved && child != new_child { - let mut new_t = match new_child { - View::Text(t) => t, - _ => unreachable!(), - }; - prev_t - .unchecked_ref::() - .set_data(&new_t.content); + // nb: the match/ownership gymnastics here + // are so that, if we can reuse the text node, + // we can take ownership of new_t so we don't clone + // the contents, which in O(n) on the length of the text + if matches!(new_child, View::Text(_)) { + if !was_child_moved && child != new_child { + let mut new_t = match new_child { + View::Text(t) => t, + _ => unreachable!(), + }; + prev_t + .unchecked_ref::() + .set_data(&new_t.content); - // replace new_t's text node with the prev node - // see discussion: https://github.com/leptos-rs/leptos/pull/1472 - new_t.node = prev_t.clone(); + // replace new_t's text node with the prev node + // see discussion: https://github.com/leptos-rs/leptos/pull/1472 + new_t.node = prev_t.clone(); - let new_child = View::Text(new_t); - **child_borrow = Some(new_child); + let new_child = View::Text(new_t); + **child_borrow = Some(new_child); - Some(prev_t) - } else { - let new_t = new_child.as_text().unwrap(); + Some(prev_t) + } else { + let new_t = new_child.as_text().unwrap(); + mount_child( + MountKind::Before(&closing), + &new_child, + ); + + **child_borrow = Some(new_child.clone()); + + Some(new_t.node.clone()) + } + } + // Child is not a text node, so we can remove the previous + // text node + else { + if !was_child_moved && child != new_child { + // Remove the text + closing + .previous_non_view_marker_sibling() + .unwrap() + .unchecked_into::() + .remove(); + } + + // Mount the new child, and we're done mount_child( MountKind::Before(&closing), &new_child, ); - **child_borrow = Some(new_child.clone()); + **child_borrow = Some(new_child); - Some(new_t.node.clone()) + None } } - // Child is not a text node, so we can remove the previous - // text node + // Otherwise, the new child can still be a text node, + // but we know the previous child was not, so no special + // treatment here else { - if !was_child_moved && child != new_child { - // Remove the text - closing - .previous_non_view_marker_sibling() - .unwrap() - .unchecked_into::() - .remove(); + // Technically, I think this check shouldn't be necessary, but + // I can imagine some edge case that the child changes while + // hydration is ongoing + if !HydrationCtx::is_hydrating() { + let same_child = child == new_child; + if !was_child_moved && !same_child { + // Remove the child + let start = child.get_opening_node(); + let end = &closing; + + match child { + View::CoreComponent( + crate::CoreComponent::DynChild( + child, + ), + ) => { + let start = + child.get_opening_node(); + let end = child.closing.node; + prepare_to_move( + &child.document_fragment, + &start, + &end, + ); + } + View::Component(child) => { + let start = + child.get_opening_node(); + let end = child.closing.node; + prepare_to_move( + &child.document_fragment, + &start, + &end, + ); + } + _ => unmount_child(&start, end), + } + } + + // Mount the new child + // If it's the same child, don't re-mount + if !same_child { + mount_child( + MountKind::Before(&closing), + &new_child, + ); + } } - // Mount the new child, and we're done + // We want to reuse text nodes, so hold onto it if + // our child is one + let t = + new_child.get_text().map(|t| t.node.clone()); + + **child_borrow = Some(new_child); + + t + }; + + ret + } + // Otherwise, we know for sure this is our first time + else { + // If it's a text node, we want to use the old text node + // as the text node for the DynChild, rather than the new + // text node being created during hydration + let new_child = if HydrationCtx::is_hydrating() + && new_child.get_text().is_some() + { + let t = closing + .previous_non_view_marker_sibling() + .unwrap() + .unchecked_into::(); + + let new_child = match new_child { + View::Text(text) => text, + _ => unreachable!(), + }; + t.set_data(&new_child.content); + View::Text(Text { + node: t.unchecked_into(), + content: new_child.content, + }) + } else { + new_child + }; + + // If we are not hydrating, we simply mount the child + if !HydrationCtx::is_hydrating() { mount_child( MountKind::Before(&closing), &new_child, ); - - **child_borrow = Some(new_child); - - None - } - } - // Otherwise, the new child can still be a text node, - // but we know the previous child was not, so no special - // treatment here - else { - // Technically, I think this check shouldn't be necessary, but - // I can imagine some edge case that the child changes while - // hydration is ongoing - if !HydrationCtx::is_hydrating() { - let same_child = child == new_child; - if !was_child_moved && !same_child { - // Remove the child - let start = child.get_opening_node(); - let end = &closing; - - match child { - View::CoreComponent( - crate::CoreComponent::DynChild(child), - ) => { - let start = child.get_opening_node(); - let end = child.closing.node; - prepare_to_move( - &child.document_fragment, - &start, - &end, - ); - } - View::Component(child) => { - let start = child.get_opening_node(); - let end = child.closing.node; - prepare_to_move( - &child.document_fragment, - &start, - &end, - ); - } - _ => unmount_child(&start, end), - } - } - - // Mount the new child - // If it's the same child, don't re-mount - if !same_child { - mount_child( - MountKind::Before(&closing), - &new_child, - ); - } } - // We want to reuse text nodes, so hold onto it if - // our child is one + // We want to update text nodes, rather than replace them, so + // make sure to hold onto the text node let t = new_child.get_text().map(|t| t.node.clone()); **child_borrow = Some(new_child); t - }; - - ret - } - // Otherwise, we know for sure this is our first time - else { - // If it's a text node, we want to use the old text node - // as the text node for the DynChild, rather than the new - // text node being created during hydration - let new_child = if HydrationCtx::is_hydrating() - && new_child.get_text().is_some() - { - let t = closing - .previous_non_view_marker_sibling() - .unwrap() - .unchecked_into::(); - - let new_child = match new_child { - View::Text(text) => text, - _ => unreachable!(), - }; - t.set_data(&new_child.content); - View::Text(Text { - node: t.unchecked_into(), - content: new_child.content, - }) - } else { - new_child - }; - - // If we are not hydrating, we simply mount the child - if !HydrationCtx::is_hydrating() { - mount_child(MountKind::Before(&closing), &new_child); } - - // We want to update text nodes, rather than replace them, so - // make sure to hold onto the text node - let t = new_child.get_text().map(|t| t.node.clone()); - - **child_borrow = Some(new_child); - - t - } - }); + }, + ); #[cfg(not(all(target_arch = "wasm32", feature = "web")))] { diff --git a/leptos_dom/src/components/each.rs b/leptos_dom/src/components/each.rs index 7cb987513..6ae9676b9 100644 --- a/leptos_dom/src/components/each.rs +++ b/leptos_dom/src/components/each.rs @@ -12,7 +12,7 @@ mod web { mount_child, prepare_to_move, MountKind, Mountable, RANGE, }; pub use drain_filter_polyfill::VecExt as VecDrainFilterExt; - pub use leptos_reactive::create_effect; + pub use leptos_reactive::create_render_effect; pub use std::cell::OnceCell; pub use wasm_bindgen::JsCast; } @@ -406,90 +406,100 @@ where let each_fn = as_child_of_current_owner(each_fn); #[cfg(all(target_arch = "wasm32", feature = "web"))] - create_effect(move |prev_hash_run: Option>>| { - let mut children_borrow = children.borrow_mut(); + create_render_effect( + move |prev_hash_run: Option>>| { + let mut children_borrow = children.borrow_mut(); - #[cfg(all( - not(debug_assertions), - target_arch = "wasm32", - feature = "web" - ))] - let opening = if let Some(Some(child)) = children_borrow.get(0) { - // correctly remove opening - let child_opening = child.get_opening_node(); - #[cfg(debug_assertions)] + #[cfg(all( + not(debug_assertions), + target_arch = "wasm32", + feature = "web" + ))] + let opening = if let Some(Some(child)) = children_borrow.get(0) { - use crate::components::dyn_child::NonViewMarkerSibling; - child_opening - .previous_non_view_marker_sibling() - .unwrap_or(child_opening) + // correctly remove opening + let child_opening = child.get_opening_node(); + #[cfg(debug_assertions)] + { + use crate::components::dyn_child::NonViewMarkerSibling; + child_opening + .previous_non_view_marker_sibling() + .unwrap_or(child_opening) + } + #[cfg(not(debug_assertions))] + { + child_opening + } + } else { + closing.clone() + }; + + let items_iter = items_fn().into_iter(); + + let (capacity, _) = items_iter.size_hint(); + let mut hashed_items = FxIndexSet::with_capacity_and_hasher( + capacity, + Default::default(), + ); + + if let Some(HashRun(prev_hash_run)) = prev_hash_run { + if !prev_hash_run.is_empty() { + let mut items = Vec::with_capacity(capacity); + for item in items_iter { + hashed_items.insert(key_fn(&item)); + items.push(Some(item)); + } + + let cmds = diff(&prev_hash_run, &hashed_items); + + apply_diff( + #[cfg(all( + target_arch = "wasm32", + feature = "web" + ))] + &opening, + #[cfg(all( + target_arch = "wasm32", + feature = "web" + ))] + &closing, + cmds, + &mut children_borrow, + items, + &each_fn, + ); + return HashRun(hashed_items); + } } - #[cfg(not(debug_assertions))] - { - child_opening - } - } else { - closing.clone() - }; - let items_iter = items_fn().into_iter(); + // if previous run is empty + *children_borrow = Vec::with_capacity(capacity); + #[cfg(all(target_arch = "wasm32", feature = "web"))] + let fragment = crate::document().create_document_fragment(); - let (capacity, _) = items_iter.size_hint(); - let mut hashed_items = FxIndexSet::with_capacity_and_hasher( - capacity, - Default::default(), - ); + for item in items_iter { + hashed_items.insert(key_fn(&item)); + let (child, disposer) = each_fn(item); + let each_item = EachItem::new(disposer, child.into_view()); - if let Some(HashRun(prev_hash_run)) = prev_hash_run { - if !prev_hash_run.is_empty() { - let mut items = Vec::with_capacity(capacity); - for item in items_iter { - hashed_items.insert(key_fn(&item)); - items.push(Some(item)); + #[cfg(all(target_arch = "wasm32", feature = "web"))] + { + _ = fragment + .append_child(&each_item.get_mountable_node()); } - let cmds = diff(&prev_hash_run, &hashed_items); - - apply_diff( - #[cfg(all(target_arch = "wasm32", feature = "web"))] - &opening, - #[cfg(all(target_arch = "wasm32", feature = "web"))] - &closing, - cmds, - &mut children_borrow, - items, - &each_fn, - ); - return HashRun(hashed_items); + children_borrow.push(Some(each_item)); } - } - - // if previous run is empty - *children_borrow = Vec::with_capacity(capacity); - #[cfg(all(target_arch = "wasm32", feature = "web"))] - let fragment = crate::document().create_document_fragment(); - - for item in items_iter { - hashed_items.insert(key_fn(&item)); - let (child, disposer) = each_fn(item); - let each_item = EachItem::new(disposer, child.into_view()); #[cfg(all(target_arch = "wasm32", feature = "web"))] - { - _ = fragment.append_child(&each_item.get_mountable_node()); - } + closing + .unchecked_ref::() + .before_with_node_1(&fragment) + .expect("before to not err"); - children_borrow.push(Some(each_item)); - } - - #[cfg(all(target_arch = "wasm32", feature = "web"))] - closing - .unchecked_ref::() - .before_with_node_1(&fragment) - .expect("before to not err"); - - HashRun(hashed_items) - }); + HashRun(hashed_items) + }, + ); #[cfg(not(all(target_arch = "wasm32", feature = "web")))] { diff --git a/leptos_dom/src/html.rs b/leptos_dom/src/html.rs index d82e1e4e8..75e728d34 100644 --- a/leptos_dom/src/html.rs +++ b/leptos_dom/src/html.rs @@ -774,7 +774,7 @@ impl HtmlElement { let class_list = self.element.as_ref().class_list(); - leptos_reactive::create_effect( + leptos_reactive::create_render_effect( move |prev_classes: Option< SmallVec<[Oco<'static, str>; 4]>, >| { diff --git a/leptos_dom/src/node_ref.rs b/leptos_dom/src/node_ref.rs index 32adc6015..cc923a86c 100644 --- a/leptos_dom/src/node_ref.rs +++ b/leptos_dom/src/node_ref.rs @@ -1,6 +1,6 @@ use crate::{html::ElementDescriptor, HtmlElement}; use leptos_reactive::{ - create_effect, create_rw_signal, signal_prelude::*, RwSignal, + create_render_effect, create_rw_signal, signal_prelude::*, RwSignal, }; use std::cell::Cell; @@ -134,7 +134,7 @@ impl NodeRef { { let f = Cell::new(Some(f)); - create_effect(move |_| { + create_render_effect(move |_| { if let Some(node_ref) = self.get() { f.take().unwrap()(node_ref); } diff --git a/leptos_reactive/src/effect.rs b/leptos_reactive/src/effect.rs index 8ba3e93c6..3062d3c2e 100644 --- a/leptos_reactive/src/effect.rs +++ b/leptos_reactive/src/effect.rs @@ -3,15 +3,22 @@ use cfg_if::cfg_if; use std::{any::Any, cell::RefCell, marker::PhantomData, rc::Rc}; /// Effects run a certain chunk of code whenever the signals they depend on change. -/// `create_effect` immediately runs the given function once, tracks its dependence +/// `create_effect` queues the given function to run once, tracks its dependence /// on any signal values read within it, and reruns the function whenever the value /// of a dependency changes. /// /// Effects are intended to run *side-effects* of the system, not to synchronize state -/// *within* the system. In other words: don't write to signals within effects. +/// *within* the system. In other words: don't write to signals within effects, unless +/// you’re coordinating with some other non-reactive side effect. /// (If you need to define a signal that depends on the value of other signals, use a /// derived signal or [`create_memo`](crate::create_memo)). /// +/// This first run is queued for the next microtask, i.e., it runs after all other +/// synchronous code has completed. In practical terms, this means that if you use +/// `create_effect` in the body of the component, it will run *after* the view has been +/// created and (presumably) mounted. (If you need an effect that runs immediately, use +/// [`create_render_effect`].) +/// /// The effect function is called with an argument containing whatever value it returned /// the last time it ran. On the initial run, this is `None`. /// @@ -63,12 +70,20 @@ where { cfg_if! { if #[cfg(not(feature = "ssr"))] { + use crate::{Owner, queue_microtask, with_owner}; + let runtime = Runtime::current(); + let owner = Owner::current(); let id = runtime.create_effect(f); - //crate::macros::debug_warn!("creating effect {e:?}"); - _ = with_runtime( |runtime| { - runtime.update_if_necessary(id); + + queue_microtask(move || { + with_owner(owner.unwrap(), move || { + _ = with_runtime( |runtime| { + runtime.update_if_necessary(id); + }); + }); }); + Effect { id, ty: PhantomData } } else { // clear warnings @@ -246,7 +261,10 @@ where } } -#[doc(hidden)] +/// Creates an effect exactly like [`create_effect`], but runs immediately rather +/// than being queued until the end of the current microtask. This is mostly used +/// inside the renderer but is available for use cases in which scheduling the effect +/// for the next tick is not optimal. #[cfg_attr( any(debug_assertions, feature="ssr"), instrument( @@ -258,11 +276,26 @@ where ) )] #[inline(always)] -pub fn create_render_effect(f: impl Fn(Option) -> T + 'static) +pub fn create_render_effect( + f: impl Fn(Option) -> T + 'static, +) -> Effect where T: 'static, { - create_effect(f); + cfg_if! { + if #[cfg(not(feature = "ssr"))] { + let runtime = Runtime::current(); + let id = runtime.create_effect(f); + _ = with_runtime( |runtime| { + runtime.update_if_necessary(id); + }); + Effect { id, ty: PhantomData } + } else { + // clear warnings + _ = f; + Effect { id: Default::default(), ty: PhantomData } + } + } } /// A handle to an effect, can be used to explicitly dispose of the effect.