From 487dba90d83b6d8a35faac5ec6632b59dbda654b Mon Sep 17 00:00:00 2001 From: jquesada2016 <54370171+jquesada2016@users.noreply.github.com> Date: Sat, 18 Feb 2023 13:57:14 -0600 Subject: [PATCH] fix: off-by-one error in `` (closes #533) (#538) --- leptos_dom/examples/hydration-test/src/lib.rs | 90 +++---- leptos_dom/examples/test-bench/Cargo.toml | 1 + leptos_dom/examples/test-bench/src/main.rs | 116 +++------ leptos_dom/examples/test-bench/src/utils.rs | 47 ---- leptos_dom/src/components/each.rs | 230 +++++++++--------- 5 files changed, 195 insertions(+), 289 deletions(-) delete mode 100644 leptos_dom/examples/test-bench/src/utils.rs diff --git a/leptos_dom/examples/hydration-test/src/lib.rs b/leptos_dom/examples/hydration-test/src/lib.rs index 40c2fc1f5..bfc37c5cb 100644 --- a/leptos_dom/examples/hydration-test/src/lib.rs +++ b/leptos_dom/examples/hydration-test/src/lib.rs @@ -4,55 +4,55 @@ use leptos::*; #[component] pub fn App(cx: Scope) -> impl IntoView { - let pending_thing = create_resource( - cx, - || false, - |_| async { - if cfg!(feature = "ssr") { - let (tx, rx) = futures::channel::oneshot::channel(); - spawn_local(async { - std::thread::sleep(std::time::Duration::from_millis(10)); - tx.send(()); - }); - rx.await; - } else { - } - true - }, - ); + let pending_thing = create_resource( + cx, + || false, + |_| async { + if cfg!(feature = "ssr") { + let (tx, rx) = futures::channel::oneshot::channel(); + spawn_local(async { + std::thread::sleep(std::time::Duration::from_millis(10)); + tx.send(()); + }); + rx.await; + } else { + } + true + }, + ); - view! { cx, -
+ view! { cx,
- "This is some text" +
+ "This is some text" +
+ // "Loading..."

}> + {move || pending_thing.read().map(|n| view! { cx, })} + //
- // "Loading..."

}> - {move || pending_thing.read().map(|n| view! { cx, })} - // -
- } + } } #[component] pub fn ComponentA(cx: Scope) -> impl IntoView { - let (value, set_value) = create_signal(cx, "Hello?".to_string()); - let (counter, set_counter) = create_signal(cx, 0); + let (value, set_value) = create_signal(cx, "Hello?".to_string()); + let (counter, set_counter) = create_signal(cx, 0); - // Test to make sure hydration isn't broken by - // something like this - //let _ = [div(cx)].into_view(cx); + // Test to make sure hydration isn't broken by + // something like this + //let _ = [div(cx)].into_view(cx); - div(cx) - .id("the-div") - .child( - input(cx) - .attr("type", "text") - .prop("value", (cx, value)) - .on(ev::input, move |e| set_value(event_target_value(&e))), - ) - .child(input(cx).attr("type", "text").prop("value", value)) - .child(p(cx).child("Value: ").child(value)) - .into_view(cx) + div(cx) + .id("the-div") + .child( + input(cx) + .attr("type", "text") + .prop("value", (cx, value)) + .on(ev::input, move |e| set_value(event_target_value(&e))), + ) + .child(input(cx).attr("type", "text").prop("value", value)) + .child(p(cx).child("Value: ").child(value)) + .into_view(cx) } #[cfg(feature = "hydrate")] @@ -61,11 +61,11 @@ use wasm_bindgen::prelude::wasm_bindgen; #[cfg(feature = "hydrate")] #[wasm_bindgen] pub fn hydrate() { - console_error_panic_hook::set_once(); + console_error_panic_hook::set_once(); - gloo::console::debug!("starting WASM"); + gloo::console::debug!("starting WASM"); - leptos::mount_to_body(move |cx| { - view! { cx, } - }); + leptos::mount_to_body(move |cx| { + view! { cx, } + }); } diff --git a/leptos_dom/examples/test-bench/Cargo.toml b/leptos_dom/examples/test-bench/Cargo.toml index 67e132e6a..b3b7da3f9 100644 --- a/leptos_dom/examples/test-bench/Cargo.toml +++ b/leptos_dom/examples/test-bench/Cargo.toml @@ -9,6 +9,7 @@ gloo = { version = "0.8", features = ["futures"] } leptos = { path = "../../../leptos", features = ["tracing"] } tracing = "0.1" tracing-subscriber = "0.3" +tracing-subscriber-wasm = "0.1" wasm-bindgen-futures = "0.4" web-sys = "0.3" diff --git a/leptos_dom/examples/test-bench/src/main.rs b/leptos_dom/examples/test-bench/src/main.rs index c06183070..b33f06d6d 100644 --- a/leptos_dom/examples/test-bench/src/main.rs +++ b/leptos_dom/examples/test-bench/src/main.rs @@ -1,102 +1,42 @@ -#![allow(warnings)] - #[macro_use] extern crate tracing; -mod utils; - use leptos::*; -use tracing::field::debug; -use tracing_subscriber::util::SubscriberInitExt; +use tracing_subscriber::prelude::*; fn main() { - console_error_panic_hook::set_once(); + tracing_subscriber::fmt() + .with_writer(tracing_subscriber_wasm::MakeConsoleWriter::default()) + .without_time() + .with_max_level(tracing::Level::TRACE) + .pretty() + .with_target(false) + .init(); - tracing_subscriber::fmt() - .with_max_level(tracing::Level::TRACE) - .without_time() - .with_file(true) - .with_line_number(true) - .with_target(false) - .with_writer(utils::MakeConsoleWriter) - .with_ansi(false) - .pretty() - .finish() - .init(); - - mount_to_body(view_fn); + mount_to_body(app); } -fn view_fn(cx: Scope) -> impl IntoView { - let view = view! { cx, - - } - .into_view(cx); +#[instrument] +fn app(cx: Scope) -> impl IntoView { + let (data, set_data) = create_signal(cx, vec![1, 3, 5]); - let (a, set_a) = create_signal(cx, view.clone()); - let (b, set_b) = create_signal(cx, view); + let handle_change = move |_| { + set_data.update(|data| { + if [1, 3, 5] == data[..] { + *data = vec![0, 1, 2, 3, 4, 5, 6]; + } else { + *data = vec![1, 3, 5]; + } + }) + }; - let (is_a, set_is_a) = create_signal(cx, true); + view! { cx, + - let handle_toggle = move |_| { - trace!("toggling"); - if is_a() { - set_b(a()); - - set_is_a(false); - } else { - set_a(a()); - - set_is_a(true); + {i} } + /> } - }; - - let a_tag = view! { cx, }; - - view! { cx, - <> -
- -
- {a_tag} - - - - - } -} - -#[component] -fn A(cx: Scope, child: Signal) -> impl IntoView { - move || child() -} - -#[component] -fn Example(cx: Scope) -> impl IntoView { - trace!("rendering "); - - let (value, set_value) = create_signal(cx, 10); - - let memo = create_memo(cx, move |_| value() * 2); - let derived = Signal::derive(cx, move || value() * 3); - - create_effect(cx, move |_| { - trace!("logging value of derived..., {}", derived.get()); - }); - - set_timeout( - move || set_value.update(|v| *v += 1), - std::time::Duration::from_millis(50), - ); - - view! { cx, -

"Example"

- - } } diff --git a/leptos_dom/examples/test-bench/src/utils.rs b/leptos_dom/examples/test-bench/src/utils.rs deleted file mode 100644 index 2ed7c3c13..000000000 --- a/leptos_dom/examples/test-bench/src/utils.rs +++ /dev/null @@ -1,47 +0,0 @@ -pub struct MakeConsoleWriter; -use std::io::{self, Write}; -use tracing_subscriber::fmt::MakeWriter; - -impl<'a> MakeWriter<'a> for MakeConsoleWriter { - type Writer = ConsoleWriter; - - fn make_writer(&'a self) -> Self::Writer { - unimplemented!("use make_writer_for instead"); - } - - fn make_writer_for(&'a self, meta: &tracing::Metadata<'_>) -> Self::Writer { - ConsoleWriter(*meta.level(), Vec::with_capacity(256)) - } -} - -pub struct ConsoleWriter(tracing::Level, Vec); - -impl io::Write for ConsoleWriter { - fn write(&mut self, buf: &[u8]) -> io::Result { - self.1.write(buf) - } - - fn flush(&mut self) -> io::Result<()> { - use gloo::console; - use tracing::Level; - - let data = String::from_utf8(self.1.to_owned()) - .map_err(|_| io::Error::new(io::ErrorKind::InvalidData, "data not UTF-8"))?; - - match self.0 { - Level::TRACE => console::debug!(&data), - Level::DEBUG => console::debug!(&data), - Level::INFO => console::log!(&data), - Level::WARN => console::warn!(&data), - Level::ERROR => console::error!(&data), - } - - Ok(()) - } -} - -impl Drop for ConsoleWriter { - fn drop(&mut self) { - let _ = self.flush(); - } -} diff --git a/leptos_dom/src/components/each.rs b/leptos_dom/src/components/each.rs index d4d15027b..1ad09e5bb 100644 --- a/leptos_dom/src/components/each.rs +++ b/leptos_dom/src/components/each.rs @@ -347,60 +347,72 @@ where let (children, closing) = (component.children.clone(), component.closing.node.clone()); - cfg_if::cfg_if! { - if #[cfg(all(target_arch = "wasm32", feature = "web"))] { + #[cfg(all(target_arch = "wasm32", feature = "web"))] + { create_effect(cx, move |prev_hash_run| { - let mut children_borrow = children.borrow_mut(); + let mut children_borrow = children.borrow_mut(); - #[cfg(all(target_arch = "wasm32", feature = "web"))] - let opening = if let Some(Some(child)) = children_borrow.get(0) { - child.get_opening_node() - } else { - closing.clone() - }; + let opening = if let Some(Some(child)) = children_borrow.get(0) + { + child.get_opening_node() + } else { + closing.clone() + }; - let items = items_fn(); + let items = items_fn(); - let items = items.into_iter().collect::>(); + let items = items.into_iter().collect::>(); - let hashed_items = - items.iter().map(&key_fn).collect::>(); + let hashed_items = + items.iter().map(&key_fn).collect::>(); - if let Some(HashRun(prev_hash_run)) = prev_hash_run { - let cmds = diff(&prev_hash_run, &hashed_items); + if let Some(HashRun(prev_hash_run)) = prev_hash_run { + let cmds = diff(&prev_hash_run, &hashed_items); - apply_cmds( - cx, - #[cfg(all(target_arch = "wasm32", feature = "web"))] - &opening, - #[cfg(all(target_arch = "wasm32", feature = "web"))] - &closing, - cmds, - &mut children_borrow, - items.into_iter().map(|t| Some(t)).collect(), - &each_fn - ); - } else { - *children_borrow = Vec::with_capacity(items.len()); + tracing::debug!("cmds:\n{cmds:#?}"); - for item in items { - let (each_item, _) = cx.run_child_scope(|cx| EachItem::new(cx, each_fn(cx, item).into_view(cx))); + apply_cmds( + cx, + &opening, + &closing, + cmds, + &mut children_borrow, + items.into_iter().map(|t| Some(t)).collect(), + &each_fn, + ); + } else { + children_borrow.clear(); + children_borrow.reserve(items.len()); - #[cfg(all(target_arch = "wasm32", feature = "web"))] - mount_child(MountKind::Before(&closing), &each_item); + for item in items { + let (each_item, disposer) = cx.run_child_scope(|cx| { + EachItem::new(cx, each_fn(cx, item).into_view(cx)) + }); - children_borrow.push(Some(each_item)); + mount_child(MountKind::Before(&closing), &each_item); + + children_borrow.push(Some(each_item)); + } } - } - HashRun(hashed_items) + HashRun(hashed_items) }); - } else { + } + + #[cfg(not(all(target_arch = "wasm32", feature = "web")))] + { *component.children.borrow_mut() = (items_fn)() - .into_iter() - .map(|child| cx.run_child_scope(|cx| Some(EachItem::new(cx, (each_fn)(cx, child).into_view(cx)))).0) - .collect(); - } + .into_iter() + .map(|child| { + cx.run_child_scope(|cx| { + Some(EachItem::new( + cx, + (each_fn)(cx, child).into_view(cx), + )) + }) + .0 + }) + .collect(); } View::CoreComponent(CoreComponent::Each(component)) @@ -411,7 +423,7 @@ where #[educe(Debug)] struct HashRun(#[educe(Debug(ignore))] T); -/// Calculates the operations need to get from `a` to `b`. +/// Calculates the operations needed to get from `a` to `b`. #[cfg(all(target_arch = "wasm32", feature = "web"))] fn diff(from: &FxIndexSet, to: &FxIndexSet) -> Diff { if from.is_empty() && to.is_empty() { @@ -424,72 +436,73 @@ fn diff(from: &FxIndexSet, to: &FxIndexSet) -> Diff { } // Get removed items - let mut removed = from.difference(to); + let mut removed = + from.difference(to).map(|k| from.get_index_of(k).unwrap()); - let removed_cmds = removed - .clone() - .map(|k| from.get_full(k).unwrap().0) - .map(|idx| DiffOpRemove { at: idx }); + let removed_cmds = removed.clone().map(|idx| DiffOpRemove { at: idx }); // Get added items - let mut added = to.difference(from); + let mut added = to.difference(from).map(|k| to.get_index_of(k).unwrap()); - let added_cmds = - added - .clone() - .map(|k| to.get_full(k).unwrap().0) - .map(|idx| DiffOpAdd { - at: idx, - mode: Default::default(), - }); + let added_cmds = added.clone().map(|idx| DiffOpAdd { + at: idx, + mode: Default::default(), + }); - // Get moved items - let mut normalized_idx = 0; - let mut move_cmds = SmallVec::<[_; 8]>::with_capacity(to.len()); - let mut added_idx = added.next().map(|k| to.get_full(k).unwrap().0); - let mut removed_idx = removed.next().map(|k| from.get_full(k).unwrap().0); + let mut normalized_idx = 0i64; + let mut next_added_idx = added.next(); + let mut next_removed_idx = removed.next(); - for (idx, k) in to.iter().enumerate() { - if let Some(added_idx) = added_idx.as_mut().filter(|r_i| **r_i == idx) { - if let Some(next_added) = - added.next().map(|k| to.get_full(k).unwrap().0) - { - *added_idx = next_added; + let move_cmds = to + .iter() + .enumerate() + .filter_map(|(i, k)| { + let is_added = if let Some(idx) = next_added_idx { + if i == idx { + next_added_idx = added.next(); + normalized_idx -= 1; - normalized_idx = usize::wrapping_sub(normalized_idx, 1); + true + } else { + false + } + } else { + false + }; + + let is_removed = if let Some(idx) = next_removed_idx { + if i == idx { + next_removed_idx = removed.next(); + normalized_idx += 1; + + true + } else { + false + } + } else { + false + }; + + normalized_idx += 1; + + if !is_added && !is_removed { + Some(( + from.get_index_of(k).unwrap(), + i, + // We need to `-1` because otherwise, we'd be accounting for + // the NEXT iteration, not this current one + normalized_idx - 1, + )) + } else { + None } - } - - if let Some(removed_idx) = - removed_idx.as_mut().filter(|r_i| **r_i == idx) - { - normalized_idx = normalized_idx.wrapping_add(1); - - if let Some(next_removed) = - removed.next().map(|k| from.get_full(k).unwrap().0) - { - *removed_idx = next_removed; - } - } - - if let Some((from_idx, _)) = from.get_full(k) { - if from_idx != normalized_idx { - move_cmds.push(DiffOpMove { - from: from_idx, - to: idx, - move_in_dom: true, - }); - } else if from_idx != idx { - move_cmds.push(DiffOpMove { - from: from_idx, - to: idx, - move_in_dom: false, - }); - } - } - - normalized_idx = normalized_idx.wrapping_add(1); - } + }) + .map(|(from, to, normalized_idx)| DiffOpMove { + from, + to, + move_in_dom: to != normalized_idx as usize, + }) + .collect(); let mut diffs = Diff { removed: removed_cmds.collect(), @@ -621,17 +634,16 @@ fn apply_cmds( if opening.previous_sibling().is_none() && closing.next_sibling().is_none() { - let parent = closing + if let Some(parent) = closing .parent_node() - .expect("could not get closing node") - .unchecked_into::(); - parent.set_text_content(Some("")); + .map(JsCast::unchecked_into::) + { + #[cfg(debug_assertions)] + parent.append_with_node_2(opening, closing).unwrap(); - #[cfg(debug_assertions)] - parent.append_with_node_2(opening, closing).unwrap(); - - #[cfg(not(debug_assertions))] - parent.append_with_node_1(closing).unwrap(); + #[cfg(not(debug_assertions))] + parent.append_with_node_1(closing).unwrap(); + } } else { range.set_start_before(opening).unwrap(); range.set_end_before(closing).unwrap(); @@ -664,7 +676,7 @@ fn apply_cmds( for DiffOpAdd { at, mode } in cmds.added { let item = items[at].take().unwrap(); - let (each_item, _) = cx.run_child_scope(|cx| { + let (each_item, disposer) = cx.run_child_scope(|cx| { let child = each_fn(cx, item).into_view(cx); EachItem::new(cx, child) });