chore: adjust semantics of placeholders and fragments

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();

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(),
-            })));
-        }
-
-        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#"
+            }))
+        } else {
+            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#"
                 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<Item = impl IntoVNode<'a>>,
     ) -> 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();
 
-        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,
-        }))
+            Some(VNode::Fragment(VFragment {
+                children,
+                key: None,
+            }))
+        }
     }
 }
 
@@ -749,10 +753,9 @@ impl<'a> IntoVNode<'a> for Option<LazyNodes<'a, '_>> {
     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(),
+            })),
         }
     }
 }

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 },
         ]
     );
 }