From bc43069884d74fe01f77cdfbc2b492db5f9a328e Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Wed, 2 Feb 2022 19:04:37 -0500 Subject: [PATCH 1/3] fix: allow scopes and nodes to be missing --- packages/core/src/diff.rs | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/packages/core/src/diff.rs b/packages/core/src/diff.rs index 48163ccf7..2c8a66ca8 100644 --- a/packages/core/src/diff.rs +++ b/packages/core/src/diff.rs @@ -329,10 +329,11 @@ impl<'b> DiffState<'b> { old_node: &'b VNode<'b>, new_node: &'b VNode<'b>, ) { - let root = old - .id - .get() - .expect("existing element nodes should have an ElementId"); + // if the node is comming back not assigned, that means it was borrowed but removed + let root = match old.id.get() { + Some(id) => id, + None => self.scopes.reserve_node(new_node), + }; // If the element type is completely different, the element needs to be re-rendered completely // This is an optimization React makes due to how users structure their code @@ -931,12 +932,18 @@ impl<'b> DiffState<'b> { log::trace!("Replacing component x2 {:?}", old); - // we can only remove components if they are actively being diffed - if self.scope_stack.contains(&c.originator) { - log::trace!("Removing component {:?}", old); + let scope = self.scopes.get_scope(scope_id).unwrap(); + c.scope.set(None); + let props = scope.props.take().unwrap(); + c.props.borrow_mut().replace(props); + self.scopes.try_remove(scope_id).unwrap(); - self.scopes.try_remove(scope_id).unwrap(); - } + // // we can only remove components if they are actively being diffed + // if self.scope_stack.contains(&c.originator) { + // log::trace!("Removing component {:?}", old); + + // self.scopes.try_remove(scope_id).unwrap(); + // } } self.leave_scope(); } @@ -950,6 +957,7 @@ impl<'b> DiffState<'b> { // this check exists because our null node will be removed but does not have an ID if let Some(id) = t.id.get() { self.scopes.collect_garbage(id); + t.id.set(None); if gen_muts { self.mutations.remove(id.as_u64()); @@ -959,6 +967,7 @@ impl<'b> DiffState<'b> { VNode::Placeholder(a) => { let id = a.id.get().unwrap(); self.scopes.collect_garbage(id); + a.id.set(None); if gen_muts { self.mutations.remove(id.as_u64()); @@ -972,6 +981,7 @@ impl<'b> DiffState<'b> { } self.scopes.collect_garbage(id); + e.id.set(None); self.remove_nodes(e.children, false); } @@ -987,10 +997,12 @@ impl<'b> DiffState<'b> { let root = self.scopes.root_node(scope_id); self.remove_nodes([root], gen_muts); - // we can only remove this node if the originator is actively in our stackß - if self.scope_stack.contains(&c.originator) { - self.scopes.try_remove(scope_id).unwrap(); - } + let scope = self.scopes.get_scope(scope_id).unwrap(); + c.scope.set(None); + + let props = scope.props.take().unwrap(); + c.props.borrow_mut().replace(props); + self.scopes.try_remove(scope_id).unwrap(); } self.leave_scope(); } From f7b3228ab2a3bc199834b478001d37022ffd05c4 Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Wed, 2 Feb 2022 20:04:55 -0500 Subject: [PATCH 2/3] fix: enable resume node for text and placeholder --- packages/core/src/diff.rs | 108 ++++++++++++++++++++++---------------- 1 file changed, 64 insertions(+), 44 deletions(-) diff --git a/packages/core/src/diff.rs b/packages/core/src/diff.rs index 2c8a66ca8..06d8346b4 100644 --- a/packages/core/src/diff.rs +++ b/packages/core/src/diff.rs @@ -119,11 +119,15 @@ impl<'b> DiffState<'b> { pub fn diff_scope(&mut self, scopeid: ScopeId) { let (old, new) = (self.scopes.wip_head(scopeid), self.scopes.fin_head(scopeid)); - self.scope_stack.push(scopeid); let scope = self.scopes.get_scope(scopeid).unwrap(); - self.element_stack.push(scope.container); - self.diff_node(old, new); + self.scope_stack.push(scopeid); + self.element_stack.push(scope.container); + { + self.diff_node(old, new); + } + self.element_stack.pop(); + self.scope_stack.pop(); self.mutations.mark_dirty_scope(scopeid); } @@ -131,63 +135,26 @@ impl<'b> DiffState<'b> { pub fn diff_node(&mut self, old_node: &'b VNode<'b>, new_node: &'b VNode<'b>) { use VNode::{Component, Element, Fragment, Placeholder, Text}; match (old_node, new_node) { - // Check the most common cases first - // these are *actual* elements, not wrappers around lists (Text(old), Text(new)) => { - if std::ptr::eq(old, new) { - return; - } - - let root = old - .id - .get() - .expect("existing text nodes should have an ElementId"); - - if old.text != new.text { - self.mutations.set_text(new.text, root.as_u64()); - } - self.scopes.update_node(new_node, root); - - new.id.set(Some(root)); + self.diff_text_nodes(old, new, old_node, new_node); } (Placeholder(old), Placeholder(new)) => { - if std::ptr::eq(old, new) { - return; - } - - let root = old - .id - .get() - .expect("existing placeholder nodes should have an ElementId"); - - self.scopes.update_node(new_node, root); - new.id.set(Some(root)); + self.diff_placeholder_nodes(old, new, old_node, new_node); } (Element(old), Element(new)) => { - if std::ptr::eq(old, new) { - return; - } self.diff_element_nodes(old, new, old_node, new_node); } - // These two sets are pointers to nodes but are not actually nodes themselves (Component(old), Component(new)) => { - if std::ptr::eq(old, new) { - return; - } self.diff_component_nodes(old_node, new_node, *old, *new); } (Fragment(old), Fragment(new)) => { - if std::ptr::eq(old, new) { - return; - } self.diff_fragment_nodes(old, new); } - // Anything else is just a basic replace and create ( Component(_) | Fragment(_) | Text(_) | Element(_) | Placeholder(_), Component(_) | Fragment(_) | Text(_) | Element(_) | Placeholder(_), @@ -322,6 +289,53 @@ impl<'b> DiffState<'b> { created } + pub(crate) fn diff_text_nodes( + &mut self, + old: &'b VText<'b>, + new: &'b VText<'b>, + _old_node: &'b VNode<'b>, + new_node: &'b VNode<'b>, + ) { + if std::ptr::eq(old, new) { + return; + } + + // if the node is comming back not assigned, that means it was borrowed but removed + let root = match old.id.get() { + Some(id) => id, + None => self.scopes.reserve_node(new_node), + }; + + if old.text != new.text { + self.mutations.set_text(new.text, root.as_u64()); + } + + self.scopes.update_node(new_node, root); + + new.id.set(Some(root)); + } + + pub(crate) fn diff_placeholder_nodes( + &mut self, + old: &'b VPlaceholder, + new: &'b VPlaceholder, + _old_node: &'b VNode<'b>, + new_node: &'b VNode<'b>, + ) { + if std::ptr::eq(old, new) { + return; + } + + // if the node is comming back not assigned, that means it was borrowed but removed + let root = match old.id.get() { + Some(id) => id, + None => self.scopes.reserve_node(new_node), + }; + + self.scopes.update_node(new_node, root); + new.id.set(Some(root)); + } + fn diff_element_nodes( &mut self, old: &'b VElement<'b>, @@ -329,6 +343,10 @@ impl<'b> DiffState<'b> { old_node: &'b VNode<'b>, new_node: &'b VNode<'b>, ) { + if std::ptr::eq(old, new) { + return; + } + // if the node is comming back not assigned, that means it was borrowed but removed let root = match old.id.get() { Some(id) => id, @@ -490,6 +508,10 @@ impl<'b> DiffState<'b> { } fn diff_fragment_nodes(&mut self, old: &'b VFragment<'b>, new: &'b VFragment<'b>) { + if std::ptr::eq(old, new) { + return; + } + // This is the case where options or direct vnodes might be used. // In this case, it's faster to just skip ahead to their diff if old.children.len() == 1 && new.children.len() == 1 { @@ -1106,9 +1128,7 @@ impl<'b> DiffState<'b> { for child in el.children.iter() { num_on_stack += self.push_all_nodes(child); } - self.mutations.push_root(el.id.get().unwrap()); - num_on_stack + 1 } } From 1913ccd61f24d3b23aecc0fe3151716fec2ea1ec Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Thu, 3 Feb 2022 00:35:10 -0500 Subject: [PATCH 3/3] chore: current_scope always panics --- packages/core/src/diff.rs | 44 +++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 23 deletions(-) diff --git a/packages/core/src/diff.rs b/packages/core/src/diff.rs index 06d8346b4..d7189a710 100644 --- a/packages/core/src/diff.rs +++ b/packages/core/src/diff.rs @@ -208,9 +208,7 @@ impl<'b> DiffState<'b> { { self.mutations.create_element(tag_name, *namespace, real_id); - let cur_scope_id = self - .current_scope() - .expect("diffing should always have a scope"); + let cur_scope_id = self.current_scope(); for listener in listeners.iter() { listener.mounted_node.set(Some(real_id)); @@ -235,7 +233,7 @@ impl<'b> DiffState<'b> { } fn create_component_node(&mut self, vcomponent: &'b VComponent<'b>) -> usize { - let parent_idx = self.current_scope().unwrap(); + let parent_idx = self.current_scope(); // the component might already exist - if it does, we need to reuse it // this makes figure out when to drop the component more complicated @@ -401,25 +399,25 @@ impl<'b> DiffState<'b> { // We also need to make sure that all listeners are properly attached to the parent scope (fix_listener) // // TODO: take a more efficient path than this - if let Some(cur_scope_id) = self.current_scope() { - if old.listeners.len() == new.listeners.len() { - for (old_l, new_l) in old.listeners.iter().zip(new.listeners.iter()) { - if old_l.event != new_l.event { - self.mutations - .remove_event_listener(old_l.event, root.as_u64()); - self.mutations.new_event_listener(new_l, cur_scope_id); - } - new_l.mounted_node.set(old_l.mounted_node.get()); - } - } else { - for listener in old.listeners { + let cur_scope_id = self.current_scope(); + + if old.listeners.len() == new.listeners.len() { + for (old_l, new_l) in old.listeners.iter().zip(new.listeners.iter()) { + if old_l.event != new_l.event { self.mutations - .remove_event_listener(listener.event, root.as_u64()); - } - for listener in new.listeners { - listener.mounted_node.set(Some(root)); - self.mutations.new_event_listener(listener, cur_scope_id); + .remove_event_listener(old_l.event, root.as_u64()); + self.mutations.new_event_listener(new_l, cur_scope_id); } + new_l.mounted_node.set(old_l.mounted_node.get()); + } + } else { + for listener in old.listeners { + self.mutations + .remove_event_listener(listener.event, root.as_u64()); + } + for listener in new.listeners { + listener.mounted_node.set(Some(root)); + self.mutations.new_event_listener(listener, cur_scope_id); } } @@ -1057,8 +1055,8 @@ impl<'b> DiffState<'b> { self.mutations.insert_before(first, created as u32); } - fn current_scope(&self) -> Option { - self.scope_stack.last().copied() + fn current_scope(&self) -> ScopeId { + self.scope_stack.last().copied().expect("no current scope") } fn enter_scope(&mut self, scope: ScopeId) {