diff --git a/packages/core/Cargo.toml b/packages/core/Cargo.toml index 03c120301..af134b8ce 100644 --- a/packages/core/Cargo.toml +++ b/packages/core/Cargo.toml @@ -41,6 +41,7 @@ log = "0.4.17" [dev-dependencies] tokio = { version = "*", features = ["full"] } dioxus = { path = "../dioxus" } +pretty_assertions = "1.3.0" [features] default = [] diff --git a/packages/core/src/arena.rs b/packages/core/src/arena.rs index e876f09e1..5702a9c9b 100644 --- a/packages/core/src/arena.rs +++ b/packages/core/src/arena.rs @@ -115,10 +115,14 @@ impl VirtualDom { nodes.iter().for_each(|node| self.drop_scope_inner(node)) } DynamicNode::Placeholder(t) => { - self.try_reclaim(t.id.get().unwrap()); + if let Some(id) = t.id.get() { + self.try_reclaim(id); + } } DynamicNode::Text(t) => { - self.try_reclaim(t.id.get().unwrap()); + if let Some(id) = t.id.get() { + self.try_reclaim(id); + } } }); diff --git a/packages/core/src/diff.rs b/packages/core/src/diff.rs index 84baa5b32..7b8e33083 100644 --- a/packages/core/src/diff.rs +++ b/packages/core/src/diff.rs @@ -155,22 +155,7 @@ impl<'b> VirtualDom { // Replace components that have different render fns if left.render_fn != right.render_fn { - let created = self.create_component_node(right_template, right, idx); - let head = unsafe { - self.scopes[left.scope.get().unwrap().0] - .root_node() - .extend_lifetime_ref() - }; - let last = match head { - RenderReturn::Sync(Ok(node)) => self.find_last_element(node), - _ => todo!(), - }; - self.mutations.push(Mutation::InsertAfter { - id: last, - m: created, - }); - self.remove_component_node(left, true); - return; + return self.replace_vcomponent(right_template, right, idx, left); } // Make sure the new vcomponent has the right scopeid associated to it @@ -203,6 +188,26 @@ impl<'b> VirtualDom { }); } + fn replace_vcomponent( + &mut self, + right_template: &'b VNode<'b>, + right: &'b VComponent<'b>, + idx: usize, + left: &'b VComponent<'b>, + ) { + let m = self.create_component_node(right_template, right, idx); + + self.remove_component_node(left, true); + + // We want to optimize the replace case to use one less mutation if possible + // Since mutations are done in reverse, the last node removed will be the first in the stack + // Instead of *just* removing it, we can use the replace mutation + match self.mutations.edits.pop().unwrap() { + Mutation::Remove { id } => self.mutations.push(Mutation::ReplaceWith { id, m }), + at => panic!("Expected remove mutation from remove_node {:#?}", at), + }; + } + /// Lightly diff the two templates, checking only their roots. /// /// The goal here is to preserve any existing component state that might exist. This is to preserve some React-like @@ -712,11 +717,21 @@ impl<'b> VirtualDom { fn replace(&mut self, left: &'b VNode<'b>, right: impl IntoIterator>) { let m = self.create_children(right); - let id = self.find_last_element(left); - - self.mutations.push(Mutation::InsertAfter { id, m }); + let pre_edits = self.mutations.edits.len(); self.remove_node(left, true); + + // We should always have a remove mutation + // Eventually we don't want to generate placeholders, so this might not be true. But it's true today + assert!(self.mutations.edits.len() > pre_edits); + + // We want to optimize the replace case to use one less mutation if possible + // Since mutations are done in reverse, the last node removed will be the first in the stack + // Instead of *just* removing it, we can use the replace mutation + match self.mutations.edits.pop().unwrap() { + Mutation::Remove { id } => self.mutations.push(Mutation::ReplaceWith { id, m }), + _ => panic!("Expected remove mutation from remove_node"), + }; } fn node_to_placeholder(&mut self, l: &'b [VNode<'b>], r: &'b VPlaceholder) { @@ -725,24 +740,45 @@ impl<'b> VirtualDom { r.id.set(Some(placeholder)); - let id = self.find_last_element(&l[0]); - self.mutations .push(Mutation::CreatePlaceholder { id: placeholder }); - self.mutations.push(Mutation::InsertAfter { id, m: 1 }); - self.remove_nodes(l); + + // We want to optimize the replace case to use one less mutation if possible + // Since mutations are done in reverse, the last node removed will be the first in the stack + // Instead of *just* removing it, we can use the replace mutation + match self.mutations.edits.pop().unwrap() { + Mutation::Remove { id } => self.mutations.push(Mutation::ReplaceWith { id, m: 1 }), + _ => panic!("Expected remove mutation from remove_node"), + }; } /// Remove these nodes from the dom /// Wont generate mutations for the inner nodes fn remove_nodes(&mut self, nodes: &'b [VNode<'b>]) { - nodes.iter().for_each(|node| self.remove_node(node, true)); + nodes + .iter() + .rev() + .for_each(|node| self.remove_node(node, true)); } fn remove_node(&mut self, node: &'b VNode<'b>, gen_muts: bool) { + // Clean up any attributes that have claimed a static node as dynamic for mount/unmounta + // Will not generate mutations! + self.reclaim_attributes(node); + + // Remove the nested dynamic nodes + // We don't generate mutations for these, as they will be removed by the parent (in the next line) + // But we still need to make sure to reclaim them from the arena and drop their hooks, etc + self.remove_nested_dyn_nodes(node); + // Clean up the roots, assuming we need to generate mutations for these + // This is done last in order to preserve Node ID reclaim order (reclaim in reverse order of claim) + self.reclaim_roots(node, gen_muts); + } + + fn reclaim_roots(&mut self, node: &VNode, gen_muts: bool) { for (idx, _) in node.template.roots.iter().enumerate() { if let Some(dy) = node.dynamic_root(idx) { self.remove_dynamic_node(dy, gen_muts); @@ -754,17 +790,9 @@ impl<'b> VirtualDom { self.reclaim(id); } } + } - for (idx, dyn_node) in node.dynamic_nodes.iter().enumerate() { - // Roots are cleaned up automatically above - if node.template.node_paths[idx].len() == 1 { - continue; - } - - self.remove_dynamic_node(dyn_node, false); - } - - // we clean up nodes with dynamic attributes, provided the node is unique and not a root node + fn reclaim_attributes(&mut self, node: &VNode) { let mut id = None; for (idx, attr) in node.dynamic_attrs.iter().enumerate() { // We'll clean up the root nodes either way, so don't worry @@ -784,49 +812,69 @@ impl<'b> VirtualDom { } } + fn remove_nested_dyn_nodes(&mut self, node: &VNode) { + for (idx, dyn_node) in node.dynamic_nodes.iter().enumerate() { + // Roots are cleaned up automatically above + if node.template.node_paths[idx].len() == 1 { + continue; + } + + self.remove_dynamic_node(dyn_node, false); + } + } + fn remove_dynamic_node(&mut self, node: &DynamicNode, gen_muts: bool) { match node { Component(comp) => self.remove_component_node(comp, gen_muts), - Text(t) => self.remove_text_node(t), - Placeholder(t) => self.remove_placeholder(t), + Text(t) => self.remove_text_node(t, gen_muts), + Placeholder(t) => self.remove_placeholder(t, gen_muts), Fragment(nodes) => nodes .iter() .for_each(|node| self.remove_node(node, gen_muts)), }; } - fn remove_placeholder(&mut self, t: &VPlaceholder) { + fn remove_placeholder(&mut self, t: &VPlaceholder, gen_muts: bool) { if let Some(id) = t.id.take() { + if gen_muts { + self.mutations.push(Mutation::Remove { id }); + } self.reclaim(id) } } - fn remove_text_node(&mut self, t: &VText) { + fn remove_text_node(&mut self, t: &VText, gen_muts: bool) { if let Some(id) = t.id.take() { + if gen_muts { + self.mutations.push(Mutation::Remove { id }); + } self.reclaim(id) } } fn remove_component_node(&mut self, comp: &VComponent, gen_muts: bool) { - if let Some(scope) = comp.scope.take() { - match unsafe { self.scopes[scope.0].root_node().extend_lifetime_ref() } { - RenderReturn::Sync(Ok(t)) => self.remove_node(t, gen_muts), - _ => todo!("cannot handle nonstandard nodes"), - }; + let scope = comp.scope.take().unwrap(); - let props = self.scopes[scope.0].props.take(); + match unsafe { self.scopes[scope.0].root_node().extend_lifetime_ref() } { + RenderReturn::Sync(Ok(t)) => { + println!("Removing component node sync {:?}", gen_muts); + self.remove_node(t, gen_muts) + } + _ => todo!("cannot handle nonstandard nodes"), + }; - self.dirty_scopes.remove(&DirtyScope { - height: self.scopes[scope.0].height, - id: scope, - }); + let props = self.scopes[scope.0].props.take(); - *comp.props.borrow_mut() = unsafe { std::mem::transmute(props) }; + self.dirty_scopes.remove(&DirtyScope { + height: self.scopes[scope.0].height, + id: scope, + }); - // make sure to wipe any of its props and listeners - self.ensure_drop_safety(scope); - self.scopes.remove(scope.0); - } + *comp.props.borrow_mut() = unsafe { std::mem::transmute(props) }; + + // make sure to wipe any of its props and listeners + self.ensure_drop_safety(scope); + self.scopes.remove(scope.0); } fn find_first_element(&self, node: &'b VNode<'b>) -> ElementId { diff --git a/packages/core/tests/attr_cleanup.rs b/packages/core/tests/attr_cleanup.rs index c0aa9a1da..8889f77c1 100644 --- a/packages/core/tests/attr_cleanup.rs +++ b/packages/core/tests/attr_cleanup.rs @@ -46,7 +46,7 @@ fn attrs_cycle() { assert_eq!( dom.render_immediate().santize().edits, [ - LoadTemplate { name: "template", index: 0, id: ElementId(3) }, + LoadTemplate { name: "template", index: 0, id: ElementId(1) }, ReplaceWith { id: ElementId(2), m: 1 } ] ); @@ -56,10 +56,10 @@ fn attrs_cycle() { dom.render_immediate().santize().edits, [ LoadTemplate { name: "template", index: 0, id: ElementId(2) }, - AssignId { path: &[0], id: ElementId(1) }, - SetAttribute { name: "class", value: "3", id: ElementId(1), ns: None }, - SetAttribute { name: "id", value: "3", id: ElementId(1), ns: None }, - ReplaceWith { id: ElementId(3), m: 1 } + AssignId { path: &[0], id: ElementId(3) }, + SetAttribute { name: "class", value: "3", id: ElementId(3), ns: None }, + SetAttribute { name: "id", value: "3", id: ElementId(3), ns: None }, + ReplaceWith { id: ElementId(1), m: 1 } ] ); diff --git a/packages/core/tests/diff_unkeyed_list.rs b/packages/core/tests/diff_unkeyed_list.rs index 415aa79c0..09bf3b215 100644 --- a/packages/core/tests/diff_unkeyed_list.rs +++ b/packages/core/tests/diff_unkeyed_list.rs @@ -1,5 +1,6 @@ use dioxus::core::{ElementId, Mutation::*}; use dioxus::prelude::*; +use pretty_assertions::assert_eq; #[test] fn list_creates_one_by_one() { @@ -125,7 +126,7 @@ fn removes_one_by_one() { assert_eq!( dom.render_immediate().santize().edits, [ - CreatePlaceholder { id: ElementId(3) }, + CreatePlaceholder { id: ElementId(4) }, ReplaceWith { id: ElementId(2), m: 1 } ] ); @@ -137,12 +138,12 @@ fn removes_one_by_one() { dom.render_immediate().santize().edits, [ LoadTemplate { name: "template", index: 0, id: ElementId(2) }, - HydrateText { path: &[0], value: "0", id: ElementId(4) }, + HydrateText { path: &[0], value: "0", id: ElementId(3) }, LoadTemplate { name: "template", index: 0, id: ElementId(5) }, HydrateText { path: &[0], value: "1", id: ElementId(6) }, LoadTemplate { name: "template", index: 0, id: ElementId(7) }, HydrateText { path: &[0], value: "2", id: ElementId(8) }, - ReplaceWith { id: ElementId(3), m: 3 } + ReplaceWith { id: ElementId(4), m: 3 } ] ); } @@ -264,9 +265,9 @@ fn removes_one_by_one_multiroot() { assert_eq!( dom.render_immediate().santize().edits, [ - Remove { id: ElementId(4) }, - CreatePlaceholder { id: ElementId(5) }, - ReplaceWith { id: ElementId(2), m: 1 } + CreatePlaceholder { id: ElementId(8) }, + Remove { id: ElementId(2) }, + ReplaceWith { id: ElementId(4), m: 1 } ] ); } @@ -357,11 +358,11 @@ fn remove_many() { assert_eq!( edits.edits, [ + CreatePlaceholder { id: ElementId(11,) }, Remove { id: ElementId(9,) }, Remove { id: ElementId(7,) }, Remove { id: ElementId(5,) }, Remove { id: ElementId(1,) }, - CreatePlaceholder { id: ElementId(3,) }, ReplaceWith { id: ElementId(2,), m: 1 }, ] ); @@ -372,8 +373,8 @@ fn remove_many() { edits.edits, [ LoadTemplate { name: "template", index: 0, id: ElementId(2,) }, - HydrateText { path: &[0,], value: "hello 0", id: ElementId(1,) }, - ReplaceWith { id: ElementId(3,), m: 1 }, + HydrateText { path: &[0,], value: "hello 0", id: ElementId(3,) }, + ReplaceWith { id: ElementId(11,), m: 1 }, ] ) }