From 1c516aba6a5dbe11c7a7162f3d65625e62461f03 Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Sat, 18 Dec 2021 15:49:30 -0500 Subject: [PATCH] chore: adjust semantics of placeholders and fragments --- packages/core/src/diff.rs | 55 +++++++++++++-------- packages/core/src/nodes.rs | 89 ++++++++++++++++++---------------- packages/core/tests/diffing.rs | 14 +++--- 3 files changed, 89 insertions(+), 69 deletions(-) diff --git a/packages/core/src/diff.rs b/packages/core/src/diff.rs index 5783a696e..b1d4818a2 100644 --- a/packages/core/src/diff.rs +++ b/packages/core/src/diff.rs @@ -501,9 +501,16 @@ impl<'bump> DiffState<'bump> { self.diff_component_nodes(old_node, new_node, *old, *new) } (Fragment(old), Fragment(new)) => self.diff_fragment_nodes(old, new), - (Placeholder(old), Placeholder(new)) => new.dom_id.set(old.dom_id.get()), + (Placeholder(old), Placeholder(new)) => self.diff_placeholder_nodes(old, new), (Element(old), Element(new)) => self.diff_element_nodes(old, new, old_node, new_node), + // The normal pathway still works, but generates slightly weird instructions + // This pathway just ensures we get create and replace + (Placeholder(_), Fragment(new)) => { + self.stack + .create_children(new.children, MountType::Replace { old: old_node }); + } + // Anything else is just a basic replace and create ( Component(_) | Fragment(_) | Text(_) | Element(_) | Placeholder(_), @@ -634,7 +641,9 @@ impl<'bump> DiffState<'bump> { } // todo: this is for the "settext" optimization - // it works, but i'm not sure if it's the direction we want to take + // it works, but i'm not sure if it's the direction we want to take right away + // I haven't benchmarked the performance imporvemenet yet. Perhaps + // we can make it a config? // match (old.children.len(), new.children.len()) { // (0, 0) => {} @@ -754,6 +763,11 @@ impl<'bump> DiffState<'bump> { self.diff_children(old.children, new.children); } + // probably will get inlined + fn diff_placeholder_nodes(&mut self, old: &'bump VPlaceholder, new: &'bump VPlaceholder) { + new.dom_id.set(old.dom_id.get()) + } + // ============================================= // Utilities for creating new diff instructions // ============================================= @@ -784,23 +798,26 @@ impl<'bump> DiffState<'bump> { (_, []) => { self.remove_nodes(old, true); } - ([VNode::Placeholder(old_anchor)], [VNode::Placeholder(new_anchor)]) => { - old_anchor.dom_id.set(new_anchor.dom_id.get()); - } - ([VNode::Placeholder(_)], _) => { - self.stack - .create_children(new, MountType::Replace { old: &old[0] }); - } - (_, [VNode::Placeholder(_)]) => { - let new: &'bump VNode<'bump> = &new[0]; - if let Some(first_old) = old.get(0) { - self.remove_nodes(&old[1..], true); - self.stack - .create_node(new, MountType::Replace { old: first_old }); - } else { - self.stack.create_node(new, MountType::Append {}); - } - } + + // todo: placeholders explicitly replace fragments + // + // ([VNode::Placeholder(old_anchor)], [VNode::Placeholder(new_anchor)]) => { + // old_anchor.dom_id.set(new_anchor.dom_id.get()); + // } + // ([VNode::Placeholder(_)], _) => { + // self.stack + // .create_children(new, MountType::Replace { old: &old[0] }); + // } + // (_, [VNode::Placeholder(_)]) => { + // let new: &'bump VNode<'bump> = &new[0]; + // if let Some(first_old) = old.get(0) { + // self.remove_nodes(&old[1..], true); + // self.stack + // .create_node(new, MountType::Replace { old: first_old }); + // } else { + // self.stack.create_node(new, MountType::Append {}); + // } + // } _ => { let new_is_keyed = new[0].key().is_some(); let old_is_keyed = old[0].key().is_some(); diff --git a/packages/core/src/nodes.rs b/packages/core/src/nodes.rs index 1f8b8b1c7..fd0fd3e3c 100644 --- a/packages/core/src/nodes.rs +++ b/packages/core/src/nodes.rs @@ -177,9 +177,11 @@ impl Debug for VNode<'_> { .debug_struct("VNode::VElement") .field("name", &el.tag_name) .field("key", &el.key) + .field("attrs", &el.attributes) + .field("children", &el.children) .finish(), VNode::Text(t) => write!(s, "VNode::VText {{ text: {} }}", t.text), - VNode::Placeholder(_) => write!(s, "VNode::VAnchor"), + VNode::Placeholder(_) => write!(s, "VNode::VPlaceholder"), VNode::Fragment(frag) => { write!(s, "VNode::VFragment {{ children: {:?} }}", frag.children) } @@ -225,6 +227,10 @@ pub struct VText<'src> { /// A list of VNodes with no single root. pub struct VFragment<'src> { pub key: Option<&'src str>, + + /// Fragments can never have zero children. Enforced by NodeFactory. + /// + /// You *can* make a fragment with no children, but it's not a valid fragment and your VDom will panic. pub children: &'src [VNode<'src>], } @@ -602,15 +608,15 @@ impl<'a> NodeFactory<'a> { } if nodes.is_empty() { - nodes.push(VNode::Placeholder(self.bump.alloc(VPlaceholder { + VNode::Placeholder(self.bump.alloc(VPlaceholder { dom_id: empty_cell(), - }))); + })) + } else { + VNode::Fragment(VFragment { + children: nodes.into_bump_slice(), + key: None, + }) } - - VNode::Fragment(VFragment { - children: nodes.into_bump_slice(), - key: None, - }) } pub fn fragment_from_iter<'b, 'c>( @@ -624,32 +630,34 @@ impl<'a> NodeFactory<'a> { } if nodes.is_empty() { - nodes.push(VNode::Placeholder(self.bump.alloc(VPlaceholder { + VNode::Placeholder(self.bump.alloc(VPlaceholder { dom_id: empty_cell(), - }))); - } + })) + } else { + let children = nodes.into_bump_slice(); - let children = nodes.into_bump_slice(); - - if cfg!(debug_assertions) && children.len() > 1 && children.last().unwrap().key().is_none() - { - // todo: make the backtrace prettier or remove it altogether - log::error!( - r#" + if cfg!(debug_assertions) + && children.len() > 1 + && children.last().unwrap().key().is_none() + { + // todo: make the backtrace prettier or remove it altogether + log::error!( + r#" Warning: Each child in an array or iterator should have a unique "key" prop. Not providing a key will lead to poor performance with lists. See docs.rs/dioxus for more information. ------------- {:?} "#, - backtrace::Backtrace::new() - ); - } + backtrace::Backtrace::new() + ); + } - VNode::Fragment(VFragment { - children, - key: None, - }) + VNode::Fragment(VFragment { + children, + key: None, + }) + } } // this isn't quite feasible yet @@ -658,28 +666,24 @@ impl<'a> NodeFactory<'a> { self, node_iter: impl IntoIterator>, ) -> Element<'a> { - let bump = self.bump; - let mut nodes = bumpalo::collections::Vec::new_in(bump); + let mut nodes = bumpalo::collections::Vec::new_in(self.bump); for node in node_iter { nodes.push(node.into_vnode(self)); } if nodes.is_empty() { - nodes.push(VNode::Placeholder(bump.alloc(VPlaceholder { + Some(VNode::Placeholder(self.bump.alloc(VPlaceholder { dom_id: empty_cell(), - }))); + }))) + } else { + let children = nodes.into_bump_slice(); + + Some(VNode::Fragment(VFragment { + children, + key: None, + })) } - - let children = nodes.into_bump_slice(); - - // TODO - // We need a dedicated path in the rsx! macro that will trigger the "you need keys" warning - - Some(VNode::Fragment(VFragment { - children, - key: None, - })) } } @@ -749,10 +753,9 @@ impl<'a> IntoVNode<'a> for Option> { fn into_vnode(self, cx: NodeFactory<'a>) -> VNode<'a> { match self { Some(lazy) => lazy.call(cx), - None => VNode::Fragment(VFragment { - children: &[], - key: None, - }), + None => VNode::Placeholder(cx.bump.alloc(VPlaceholder { + dom_id: empty_cell(), + })), } } } diff --git a/packages/core/tests/diffing.rs b/packages/core/tests/diffing.rs index a1afb8aa2..b8c86e615 100644 --- a/packages/core/tests/diffing.rs +++ b/packages/core/tests/diffing.rs @@ -184,12 +184,13 @@ fn empty_fragments_create_anchors_with_many_children() { create.edits, [CreatePlaceholder { root: 1 }, AppendChildren { many: 1 }] ); + assert_eq!( change.edits, [ CreateElement { + tag: "div", root: 2, - tag: "div" }, CreateTextNode { text: "hello: 0", @@ -197,8 +198,8 @@ fn empty_fragments_create_anchors_with_many_children() { }, AppendChildren { many: 1 }, CreateElement { + tag: "div", root: 4, - tag: "div" }, CreateTextNode { text: "hello: 1", @@ -206,15 +207,15 @@ fn empty_fragments_create_anchors_with_many_children() { }, AppendChildren { many: 1 }, CreateElement { + tag: "div", root: 6, - tag: "div" }, CreateTextNode { text: "hello: 2", root: 7 }, AppendChildren { many: 1 }, - ReplaceWith { m: 3, root: 1 } + ReplaceWith { root: 1, m: 3 } ] ); } @@ -257,13 +258,12 @@ fn many_items_become_fragment() { ] ); - // note: the ID gets reused assert_eq!( change.edits, [ - Remove { root: 3 }, - CreatePlaceholder { root: 4 }, + CreatePlaceholder { root: 5 }, ReplaceWith { root: 1, m: 1 }, + Remove { root: 3 }, ] ); }