From 04a923f83e35e5552d0a11672ce2797931de2e4e Mon Sep 17 00:00:00 2001 From: Evan Almloff Date: Thu, 22 Dec 2022 11:36:38 -0600 Subject: [PATCH] fix changing the order of dynamic nodes --- packages/core/src/create.rs | 140 ++++++++++++++++++++++++++++++------ packages/core/src/diff.rs | 46 ++++++------ packages/rsx/src/lib.rs | 22 ++++-- 3 files changed, 159 insertions(+), 49 deletions(-) diff --git a/packages/core/src/create.rs b/packages/core/src/create.rs index 65dccc482..b526cd161 100644 --- a/packages/core/src/create.rs +++ b/packages/core/src/create.rs @@ -7,11 +7,48 @@ use crate::nodes::{DynamicNode, TemplateNode}; use crate::virtual_dom::VirtualDom; use crate::{AttributeValue, ElementId, RenderReturn, ScopeId, SuspenseContext, Template}; use std::cell::Cell; -use std::iter::{Enumerate, Peekable}; +use std::iter::Peekable; use std::rc::Rc; -use std::slice; use TemplateNode::*; +fn sort_bfs(paths: &[&'static [u8]]) -> Vec<(usize, &'static [u8])> { + let mut with_indecies = paths.iter().copied().enumerate().collect::>(); + with_indecies.sort_unstable_by(|(_, a), (_, b)| { + let mut a = a.iter(); + let mut b = b.iter(); + loop { + match (a.next(), b.next()) { + (Some(a), Some(b)) => { + if a != b { + return a.cmp(b); + } + } + // The shorter path goes first + (Some(_), None) => return std::cmp::Ordering::Less, + (None, Some(_)) => return std::cmp::Ordering::Greater, + (None, None) => return std::cmp::Ordering::Equal, + } + } + }); + with_indecies +} + +#[test] +fn sorting() { + let r: [(usize, &[u8]); 5] = [ + (0, &[0, 1]), + (1, &[0, 2]), + (2, &[1, 0]), + (4, &[1, 1]), + (3, &[1, 2]), + ]; + assert_eq!( + sort_bfs(&[&[0, 1,], &[0, 2,], &[1, 0,], &[1, 2,], &[1, 1,],]), + r + ); + assert!(matches!(&[0], &[_, ..])) +} + impl<'b> VirtualDom { /// Create a new template [`VNode`] and write it to the [`Mutations`] buffer. /// @@ -41,9 +78,47 @@ impl<'b> VirtualDom { .reserve(node.template.get().roots.len()); // Walk the roots, creating nodes and assigning IDs + // nodes in an iterator of ((dynamic_node_index, sorted_index), path) // todo: adjust dynamic nodes to be in the order of roots and then leaves (ie BFS) - let mut attrs = node.template.get().attr_paths.iter().enumerate().peekable(); - let mut nodes = node.template.get().node_paths.iter().enumerate().peekable(); + #[cfg(not(debug_assertions))] + let (mut attrs, mut nodes) = ( + node.template + .get() + .attr_paths + .iter() + .copied() + .enumerate() + .peekable(), + node.template + .get() + .node_paths + .iter() + .copied() + .enumerate() + .map(|(i, path)| ((i, i), path)) + .peekable(), + ); + // If this is a debug build, we need to check that the paths are in the correct order because hot reloading can cause scrambled states + + #[cfg(debug_assertions)] + let (attrs_sorted, nodes_sorted) = { + ( + sort_bfs(node.template.get().attr_paths), + sort_bfs(node.template.get().node_paths), + ) + }; + #[cfg(debug_assertions)] + let (mut attrs, mut nodes) = { + ( + attrs_sorted.into_iter().peekable(), + nodes_sorted + .iter() + .copied() + .enumerate() + .map(|(i, (id, path))| ((id, i), path)) + .peekable(), + ) + }; node.template .get() @@ -55,7 +130,14 @@ impl<'b> VirtualDom { nodes.next().unwrap(); self.write_dynamic_root(node, *id) } - Element { .. } => self.write_element_root(node, idx, &mut attrs, &mut nodes), + Element { .. } => { + #[cfg(not(debug_assertions))] + let id = self.write_element_root(node, idx, &mut attrs, &mut nodes, &[]); + #[cfg(debug_assertions)] + let id = + self.write_element_root(node, idx, &mut attrs, &mut nodes, &nodes_sorted); + id + } Text { .. } => self.write_static_text_root(node, idx), }) .sum() @@ -72,8 +154,9 @@ impl<'b> VirtualDom { fn write_dynamic_root(&mut self, template: &'b VNode<'b>, idx: usize) -> usize { use DynamicNode::*; match &template.dynamic_nodes[idx] { - node @ Fragment(_) => self.create_dynamic_node(template, node, idx), - node @ Component { .. } => self.create_dynamic_node(template, node, idx), + node @ Component { .. } | node @ Fragment(_) => { + self.create_dynamic_node(template, node, idx) + } Placeholder(VPlaceholder { id }) => { let id = self.set_slot(template, id, idx); self.mutations.push(CreatePlaceholder { id }); @@ -105,17 +188,18 @@ impl<'b> VirtualDom { &mut self, template: &'b VNode<'b>, root_idx: usize, - dynamic_attrs: &mut Peekable>>, - dynamic_nodes: &mut Peekable>>, + dynamic_attrs: &mut Peekable>, + dynamic_nodes_iter: &mut Peekable>, + dynamic_nodes: &[(usize, &'static [u8])], ) -> usize { // Load the template root and get the ID for the node on the stack let root_on_stack = self.load_template_root(template, root_idx); // Write all the attributes below this root - self.write_attrs_on_root(dynamic_attrs, root_idx, root_on_stack, template); + self.write_attrs_on_root(dynamic_attrs, root_idx as u8, root_on_stack, template); // Load in all of the placeholder or dynamic content under this root too - self.load_placeholders(dynamic_nodes, root_idx, template); + self.load_placeholders(dynamic_nodes_iter, dynamic_nodes, root_idx as u8, template); 1 } @@ -133,18 +217,28 @@ impl<'b> VirtualDom { /// } /// } /// ``` + #[allow(unused)] fn load_placeholders( &mut self, - dynamic_nodes: &mut Peekable>>, - root_idx: usize, + dynamic_nodes_iter: &mut Peekable>, + dynamic_nodes: &[(usize, &'static [u8])], + root_idx: u8, template: &'b VNode<'b>, ) { - let (start, end) = match collect_dyn_node_range(dynamic_nodes, root_idx) { + let (start, end) = match collect_dyn_node_range(dynamic_nodes_iter, root_idx) { Some((a, b)) => (a, b), None => return, }; - for idx in (start..=end).rev() { + // If hot reloading is enabled, we need to map the sorted index to the original index of the dynamic node. If it is disabled, we can just use the sorted index + #[cfg(not(debug_assertions))] + let reversed_iter = (start..=end).rev(); + #[cfg(debug_assertions)] + let reversed_iter = (start..=end) + .rev() + .map(|sorted_index| dynamic_nodes[sorted_index].0); + + for idx in reversed_iter { let m = self.create_dynamic_node(template, &template.dynamic_nodes[idx], idx); if m > 0 { // The path is one shorter because the top node is the root @@ -156,12 +250,12 @@ impl<'b> VirtualDom { fn write_attrs_on_root( &mut self, - attrs: &mut Peekable>>, - root_idx: usize, + attrs: &mut Peekable>, + root_idx: u8, root: ElementId, node: &VNode, ) { - while let Some((mut attr_id, path)) = attrs.next_if(|(_, p)| p[0] == root_idx as u8) { + while let Some((mut attr_id, path)) = attrs.next_if(|(_, p)| p[0] == root_idx) { let id = self.assign_static_node_as_dynamic(path, root, node, attr_id); loop { @@ -456,17 +550,19 @@ impl<'b> VirtualDom { } fn collect_dyn_node_range( - dynamic_nodes: &mut Peekable>>, - root_idx: usize, + dynamic_nodes: &mut Peekable>, + root_idx: u8, ) -> Option<(usize, usize)> { let start = match dynamic_nodes.peek() { - Some((idx, p)) if p[0] == root_idx as u8 => *idx, + Some(((_, idx), [first, ..])) if *first == root_idx => *idx, _ => return None, }; let mut end = start; - while let Some((idx, p)) = dynamic_nodes.next_if(|(_, p)| p[0] == root_idx as u8) { + while let Some(((_, idx), p)) = + dynamic_nodes.next_if(|(_, p)| matches!(p, [idx, ..] if *idx == root_idx)) + { if p.len() == 1 { continue; } diff --git a/packages/core/src/diff.rs b/packages/core/src/diff.rs index 97335428a..58bb44ac6 100644 --- a/packages/core/src/diff.rs +++ b/packages/core/src/diff.rs @@ -61,6 +61,7 @@ impl<'b> VirtualDom { let prev_template = right_template.template.get(); if *template != prev_template { right_template.template.set(*template); + return self.replace(left_template, [right_template]); } } @@ -820,12 +821,17 @@ 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.get().node_paths[idx].len() == 1 { - continue; + let path_len = node + .template + .get() + .node_paths + .get(idx) + .map(|path| path.len()); + match path_len { + Some(2..) => self.remove_dynamic_node(dyn_node, false), + // Roots are cleaned up automatically above and nodes with a empty path are placeholders + _ => continue, } - - self.remove_dynamic_node(dyn_node, false); } } @@ -859,25 +865,25 @@ impl<'b> VirtualDom { } fn remove_component_node(&mut self, comp: &VComponent, gen_muts: bool) { - let scope = comp.scope.take().unwrap(); + if let Some(scope) = comp.scope.take() { + match unsafe { self.scopes[scope.0].root_node().extend_lifetime_ref() } { + RenderReturn::Sync(Some(t)) => self.remove_node(t, gen_muts), + _ => todo!("cannot handle nonstandard nodes"), + }; - match unsafe { self.scopes[scope.0].root_node().extend_lifetime_ref() } { - RenderReturn::Sync(Some(t)) => self.remove_node(t, gen_muts), - _ => todo!("cannot handle nonstandard nodes"), - }; + let props = self.scopes[scope.0].props.take(); - let props = self.scopes[scope.0].props.take(); + self.dirty_scopes.remove(&DirtyScope { + height: self.scopes[scope.0].height, + id: scope, + }); - self.dirty_scopes.remove(&DirtyScope { - height: self.scopes[scope.0].height, - id: scope, - }); + *comp.props.borrow_mut() = unsafe { std::mem::transmute(props) }; - *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); + // 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/rsx/src/lib.rs b/packages/rsx/src/lib.rs index c2573a2b9..124eccec1 100644 --- a/packages/rsx/src/lib.rs +++ b/packages/rsx/src/lib.rs @@ -374,13 +374,17 @@ impl<'a, Ctx: HotReloadingContext> DynamicContext<'a, Ctx> { | ElementAttr::CustomAttrText { .. } | ElementAttr::CustomAttrExpression { .. } | ElementAttr::EventTokens { .. } => { - let ct = match mapping { + let idx = match mapping { Some(mapping) => mapping.get_attribute_idx(&attr.attr)?, None => self.dynamic_attributes.len(), }; self.dynamic_attributes.push(attr); - self.attr_paths.push(self.current_path.clone()); - static_attrs.push(TemplateAttribute::Dynamic { id: ct }) + + if self.attr_paths.len() <= idx { + self.attr_paths.resize_with(idx + 1, Vec::new); + } + self.attr_paths[idx] = self.current_path.clone(); + static_attrs.push(TemplateAttribute::Dynamic { id: idx }) } } } @@ -414,16 +418,20 @@ impl<'a, Ctx: HotReloadingContext> DynamicContext<'a, Ctx> { | BodyNode::ForLoop(_) | BodyNode::IfChain(_) | BodyNode::Component(_) => { - let ct = match mapping { + let idx = match mapping { Some(mapping) => mapping.get_node_idx(root)?, None => self.dynamic_nodes.len(), }; self.dynamic_nodes.push(root); - self.node_paths.push(self.current_path.clone()); + + if self.node_paths.len() <= idx { + self.node_paths.resize_with(idx + 1, Vec::new); + } + self.node_paths[idx] = self.current_path.clone(); Some(match root { - BodyNode::Text(_) => TemplateNode::DynamicText { id: ct }, - _ => TemplateNode::Dynamic { id: ct }, + BodyNode::Text(_) => TemplateNode::DynamicText { id: idx }, + _ => TemplateNode::Dynamic { id: idx }, }) } }