From 9552ab6c1ccb8747ecdf469a52a108bb9fb34e9b Mon Sep 17 00:00:00 2001 From: Evan Almloff Date: Mon, 4 Sep 2023 19:10:11 -0500 Subject: [PATCH] pass event propagation test --- packages/core/src/arena.rs | 48 +++++++++++++++--------- packages/core/src/create.rs | 11 +++--- packages/core/src/diff.rs | 14 ++++++- packages/core/src/scope_arena.rs | 1 + packages/core/src/scopes.rs | 3 +- packages/core/src/virtual_dom.rs | 4 +- packages/core/tests/event_propagation.rs | 4 +- 7 files changed, 55 insertions(+), 30 deletions(-) diff --git a/packages/core/src/arena.rs b/packages/core/src/arena.rs index aacd4dbf2..a44e31f5a 100644 --- a/packages/core/src/arena.rs +++ b/packages/core/src/arena.rs @@ -25,7 +25,7 @@ pub struct ElementRef<'a> { pub(crate) path: ElementPath, // The actual template - pub(crate) template: *const VNode<'a>, + pub(crate) template: Option<*const VNode<'a>>, // The scope the element belongs to pub(crate) scope: ScopeId, @@ -42,10 +42,20 @@ impl VirtualDom { } pub(crate) fn next_element_ref(&mut self, element_ref: ElementRef) -> BubbleId { - BubbleId( + let new_id = BubbleId( self.element_refs .insert(unsafe { std::mem::transmute(element_ref) }), - ) + ); + + // Set this id to be dropped when the scope is rerun + if let Some(scope) = self.runtime.current_scope_id() { + self.scopes[scope.0] + .element_refs_to_drop + .borrow_mut() + .push(new_id); + } + + new_id } pub(crate) fn reclaim(&mut self, el: ElementId) { @@ -84,7 +94,7 @@ impl VirtualDom { } pub(crate) fn update_template_bubble(&mut self, bubble_id: BubbleId, node: *const VNode) { - self.element_refs[bubble_id.0].template = unsafe { std::mem::transmute(node) }; + self.element_refs[bubble_id.0].template = Some(unsafe { std::mem::transmute(node) }); } // Drop a scope and all its children @@ -124,10 +134,6 @@ impl VirtualDom { } fn drop_scope_inner(&mut self, node: &VNode) { - if let Some(id) = node.parent.get() { - self.element_refs.remove(id.0); - } - node.dynamic_nodes.iter().for_each(|node| match node { DynamicNode::Component(c) => { if let Some(f) = c.scope.get() { @@ -141,24 +147,30 @@ impl VirtualDom { DynamicNode::Fragment(nodes) => { nodes.iter().for_each(|node| self.drop_scope_inner(node)) } - DynamicNode::Placeholder(placeholder) => { - if let Some(id) = placeholder.parent.get() { - self.element_refs.remove(id.0); - } - } + DynamicNode::Placeholder(_) => {} DynamicNode::Text(_) => {} }); } /// Descend through the tree, removing any borrowed props and listeners - pub(crate) fn ensure_drop_safety(&self, scope_id: ScopeId) { + pub(crate) fn ensure_drop_safety(&mut self, scope_id: ScopeId) { let scope = &self.scopes[scope_id.0]; + { + // Drop all element refs that could be invalidated when the component was rerun + let mut element_refs = self.scopes[scope_id.0].element_refs_to_drop.borrow_mut(); + let element_refs_slab = &mut self.element_refs; + for element_ref in element_refs.drain(..) { + println!("Dropping element ref {:?}", element_ref); + element_refs_slab[element_ref.0].template = None; + } + } + // make sure we drop all borrowed props manually to guarantee that their drop implementation is called before we // run the hooks (which hold an &mut Reference) // recursively call ensure_drop_safety on all children - let mut props = scope.borrowed_props.borrow_mut(); - props.drain(..).for_each(|comp| { + let props = { scope.borrowed_props.borrow_mut().clone() }; + for comp in props { let comp = unsafe { &*comp }; match comp.scope.get() { Some(child) if child != scope_id => self.ensure_drop_safety(child), @@ -167,10 +179,10 @@ impl VirtualDom { if let Ok(mut props) = comp.props.try_borrow_mut() { *props = None; } - }); + } // Now that all the references are gone, we can safely drop our own references in our listeners. - let mut listeners = scope.attributes_to_drop.borrow_mut(); + let mut listeners = self.scopes[scope_id.0].attributes_to_drop.borrow_mut(); listeners.drain(..).for_each(|listener| { let listener = unsafe { &*listener }; match &listener.value { diff --git a/packages/core/src/create.rs b/packages/core/src/create.rs index 0b77b9417..759ee97d9 100644 --- a/packages/core/src/create.rs +++ b/packages/core/src/create.rs @@ -187,7 +187,7 @@ impl<'b> VirtualDom { path: ElementPath { path: &template.template.get().node_paths[idx], }, - template: template, + template: Some(template), scope: self.runtime.current_scope_id().unwrap_or(ScopeId(0)), }; self.create_dynamic_node(template_ref, node) @@ -197,7 +197,7 @@ impl<'b> VirtualDom { path: ElementPath { path: &template.template.get().node_paths[idx], }, - template: template, + template: Some(template), scope: self.runtime.current_scope_id().unwrap_or(ScopeId(0)), }; parent.set(Some(self.next_element_ref(template_ref))); @@ -286,7 +286,7 @@ impl<'b> VirtualDom { path: ElementPath { path: &template.template.get().node_paths[idx], }, - template, + template: Some(template), scope: self.runtime.current_scope_id().unwrap_or(ScopeId(0)), }; let m = self.create_dynamic_node(boundary_ref, &template.dynamic_nodes[idx]); @@ -340,7 +340,7 @@ impl<'b> VirtualDom { let path = &template.template.get().attr_paths[idx]; let element_ref = ElementRef { path: ElementPath { path }, - template, + template: Some(template), scope: self.runtime.current_scope_id().unwrap_or(ScopeId(0)), }; self.set_template(id, element_ref); @@ -546,9 +546,10 @@ impl<'b> VirtualDom { match unsafe { self.run_scope(scope).extend_lifetime_ref() } { // Create the component's root element Ready(t) => { + let created = self.create_scope(scope, t); self.assign_boundary_ref(parent, t); component.bubble_id.set(t.parent.get()); - self.create_scope(scope, t) + created } Aborted(t) => self.mount_aborted(t, parent), } diff --git a/packages/core/src/diff.rs b/packages/core/src/diff.rs index 6e53c7c3f..edaa0048e 100644 --- a/packages/core/src/diff.rs +++ b/packages/core/src/diff.rs @@ -144,7 +144,7 @@ impl<'b> VirtualDom { .enumerate() .for_each(|(dyn_node_idx, (left_node, right_node))| { let current_ref = ElementRef { - template: right_template, + template: Some(right_template), path: ElementPath { path: left_template.template.get().node_paths[dyn_node_idx], }, @@ -176,7 +176,7 @@ impl<'b> VirtualDom { right.id.set(left.id.get()); right.parent.set(left.parent.get()); // Update the template - self.update_template(left.id.get().unwrap(), parent.template); + self.update_template(left.id.get().unwrap(), parent.template.unwrap()); }, (Component(left), Component(right)) => self.diff_vcomponent(left, right, Some(parent)), (Placeholder(left), Fragment(right)) => self.replace_placeholder(left, *right, parent), @@ -227,6 +227,16 @@ impl<'b> VirtualDom { // The target scopestate still has the reference to the old props, so there's no need to update anything // This also implicitly drops the new props since they're not used if left.static_props && unsafe { old.as_ref().unwrap().memoize(new.as_ref()) } { + if let Some(bubble_id) = right.bubble_id.get() { + if let RenderReturn::Ready(new_node) = unsafe { + self.scopes[scope_id.0] + .current_frame() + .try_load_node() + .expect("Call rebuild before diffing") + } { + self.update_template_bubble(bubble_id, new_node); + } + } return; } diff --git a/packages/core/src/scope_arena.rs b/packages/core/src/scope_arena.rs index 138431740..cb51abf68 100644 --- a/packages/core/src/scope_arena.rs +++ b/packages/core/src/scope_arena.rs @@ -36,6 +36,7 @@ impl VirtualDom { borrowed_props: Default::default(), attributes_to_drop: Default::default(), + element_refs_to_drop: Default::default(), })); let context = diff --git a/packages/core/src/scopes.rs b/packages/core/src/scopes.rs index a9c8e4f1f..5cb066f41 100644 --- a/packages/core/src/scopes.rs +++ b/packages/core/src/scopes.rs @@ -2,7 +2,7 @@ use crate::{ any_props::AnyProps, any_props::VProps, bump_frame::BumpFrame, - innerlude::ErrorBoundary, + innerlude::{BubbleId, ErrorBoundary}, innerlude::{DynamicNode, EventHandler, VComponent, VText}, lazynodes::LazyNodes, nodes::{IntoAttributeValue, IntoDynNode, RenderReturn}, @@ -95,6 +95,7 @@ pub struct ScopeState { pub(crate) borrowed_props: RefCell>>, pub(crate) attributes_to_drop: RefCell>>, + pub(crate) element_refs_to_drop: RefCell>, pub(crate) props: Option>>, } diff --git a/packages/core/src/virtual_dom.rs b/packages/core/src/virtual_dom.rs index b5921fd48..c1043fc41 100644 --- a/packages/core/src/virtual_dom.rs +++ b/packages/core/src/virtual_dom.rs @@ -371,7 +371,7 @@ impl VirtualDom { // Loop through each dynamic attribute (in a depth first order) in this template before moving up to the template's parent. while let Some(el_ref) = parent_path { // safety: we maintain references of all vnodes in the element slab - let template = unsafe { &*el_ref.template }; + let template = unsafe { &*el_ref.template.unwrap() }; let node_template = template.template.get(); let target_path = el_ref.path; @@ -421,7 +421,7 @@ impl VirtualDom { // Otherwise, we just call the listener on the target element if let Some(el_ref) = parent_path { // safety: we maintain references of all vnodes in the element slab - let template = unsafe { &*el_ref.template }; + let template = unsafe { &*el_ref.template.unwrap() }; let node_template = template.template.get(); let target_path = el_ref.path; diff --git a/packages/core/tests/event_propagation.rs b/packages/core/tests/event_propagation.rs index 1fbf82c4a..2a87db175 100644 --- a/packages/core/tests/event_propagation.rs +++ b/packages/core/tests/event_propagation.rs @@ -16,7 +16,7 @@ fn events_propagate() { // break reference.... for _ in 0..5 { dom.mark_dirty(ScopeId(0)); - _ = dom.rebuild(); + _ = dom.render_immediate(); } // Lower click is registered @@ -26,7 +26,7 @@ fn events_propagate() { // break reference.... for _ in 0..5 { dom.mark_dirty(ScopeId(0)); - _ = dom.rebuild(); + _ = dom.render_immediate(); } // Stop propagation occurs