From 5713e13ff28ad6362e20f60f560a5ede29217086 Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Thu, 24 Nov 2022 09:11:27 -0500 Subject: [PATCH] feat: implement ID cycling --- packages/core/src/diff.rs | 149 ++++++++++++++---------- packages/core/src/garbage.rs | 9 +- packages/core/src/scheduler/wait.rs | 27 +++-- packages/core/tests/bubble_error.rs | 83 ++++--------- packages/core/tests/create_fragments.rs | 2 +- packages/core/tests/create_lists.rs | 2 +- packages/core/tests/cycle.rs | 51 ++++++++ packages/core/tests/diff_element.rs | 32 ++++- packages/core/tests/suspend.rs | 2 +- packages/rsx/src/lib.rs | 7 +- 10 files changed, 216 insertions(+), 148 deletions(-) create mode 100644 packages/core/tests/cycle.rs diff --git a/packages/core/src/diff.rs b/packages/core/src/diff.rs index 98d7dc56b..541167486 100644 --- a/packages/core/src/diff.rs +++ b/packages/core/src/diff.rs @@ -46,30 +46,29 @@ impl<'b: 'static> VirtualDom { fn diff_maybe_node(&mut self, left: &'b RenderReturn<'b>, right: &'b RenderReturn<'b>) { use RenderReturn::{Async, Sync}; match (left, right) { - // diff (Sync(Ok(l)), Sync(Ok(r))) => self.diff_node(l, r), - _ => todo!("handle diffing nonstandard nodes"), - // // remove old with placeholder - // (Sync(Ok(l)), Sync(None)) | (Sync(Ok(l)), Async(_)) => { - // // - // let id = self.next_element(l, &[]); // todo! - // m.push(Mutation::CreatePlaceholder { id }); - // self.drop_template(m, l, true); - // } + // Err cases + (Sync(Ok(l)), Sync(Err(e))) => self.diff_ok_to_err(l, e), + (Sync(Err(e)), Sync(Ok(r))) => self.diff_err_to_ok(e, r), + (Sync(Err(_eo)), Sync(Err(_en))) => { /* nothing */ } - // // remove placeholder with nodes - // (Sync(None), Sync(Ok(_))) => {} - // (Async(_), Sync(Ok(v))) => {} - - // // nothing... just transfer the placeholders over - // (Async(_), Async(_)) - // | (Sync(None), Sync(None)) - // | (Sync(None), Async(_)) - // | (Async(_), Sync(None)) => {} + // Async + (Sync(Ok(_l)), Async(_)) => todo!(), + (Sync(Err(_e)), Async(_)) => todo!(), + (Async(_), Sync(Ok(_r))) => todo!(), + (Async(_), Sync(Err(_e))) => { /* nothing */ } + (Async(_), Async(_)) => { /* nothing */ } } } + fn diff_ok_to_err(&mut self, l: &'b VNode<'b>, e: &anyhow::Error) { + todo!("Not yet handling error rollover") + } + fn diff_err_to_ok(&mut self, e: &anyhow::Error, l: &'b VNode<'b>) { + todo!("Not yet handling error rollover") + } + pub fn diff_node(&mut self, left_template: &'b VNode<'b>, right_template: &'b VNode<'b>) { if left_template.template.id != right_template.template.id { return self.light_diff_templates(left_template, right_template); @@ -97,7 +96,7 @@ impl<'b: 'static> VirtualDom { }); } // todo: more types of attribute values - _ => (), + _ => todo!("other attribute types"), } } } @@ -111,7 +110,7 @@ impl<'b: 'static> VirtualDom { (Text(left), Text(right)) => self.diff_vtext(left, right), (Fragment(left), Fragment(right)) => self.diff_vfragment(left, right), (Component(left), Component(right)) => self.diff_vcomponent(left, right), - _ => self.replace(left_template, right_template, left_node, right_node), + _ => self.replace(left_template, right_template), }; } @@ -196,7 +195,7 @@ impl<'b: 'static> VirtualDom { /// ``` fn light_diff_templates(&mut self, left: &'b VNode<'b>, right: &'b VNode<'b>) { match matching_components(left, right) { - None => self.replace_template(left, right), + None => self.replace(left, right), Some(components) => components .into_iter() .for_each(|(l, r)| self.diff_vcomponent(l, r)), @@ -236,63 +235,98 @@ impl<'b: 'static> VirtualDom { } fn replace_nodes_with_placeholder(&mut self, l: &'b [VNode<'b>], r: &'b Cell) { - // - // Remove the old nodes, except for one + // Remove the old nodes, except for onea self.remove_nodes(&l[1..]); - let first = self.find_first_element(&l[0]); - self.remove_all_but_first_node(&l[0]); + let first = self.replace_inner(&l[0]); let placeholder = self.next_element(&l[0], &[]); r.set(placeholder); + self.mutations .push(Mutation::CreatePlaceholder { id: placeholder }); self.mutations - .push(Mutation::ReplaceWith { id: first, m: 1 }) + .push(Mutation::ReplaceWith { id: first, m: 1 }); + + self.reclaim(first); } - // Remove all the top-level nodes, returning the - fn remove_all_but_first_node(&mut self, node: &'b VNode<'b>) { - match &node.template.roots[0] { - TemplateNode::Text(_) | TemplateNode::Element { .. } => {} + // Remove all the top-level nodes, returning the firstmost root ElementId + fn replace_inner(&mut self, node: &'b VNode<'b>) -> ElementId { + let id = match &node.template.roots[0] { + TemplateNode::Text(_) | TemplateNode::Element { .. } => node.root_ids[0].get(), TemplateNode::DynamicText(id) | TemplateNode::Dynamic(id) => { match &node.dynamic_nodes[*id] { - Text(_) => {} - Fragment(VFragment::Empty(_)) => {} + Text(t) => t.id.get(), + Fragment(VFragment::Empty(e)) => e.get(), Fragment(VFragment::NonEmpty(nodes)) => { - self.remove_all_but_first_node(&nodes[0]); + let id = self.replace_inner(&nodes[0]); self.remove_nodes(&nodes[1..]); + id } Component(comp) => { let scope = comp.scope.get().unwrap(); match unsafe { self.scopes[scope.0].root_node().extend_lifetime_ref() } { - RenderReturn::Sync(Ok(t)) => self.remove_all_but_first_node(t), + RenderReturn::Sync(Ok(t)) => self.replace_inner(t), _ => todo!("cannot handle nonstandard nodes"), - }; + } } - }; + } } - } + }; - // Just remove the rest + // Just remove the rest from the dom for (idx, _) in node.template.roots.iter().enumerate().skip(1) { self.remove_root_node(node, idx); } + + // Garabge collect all of the nodes since this gets used in replace + self.clean_up_node(node); + + id + } + + /// Clean up the node, not generating mutations + /// + /// Simply walks through the dynamic nodes + fn clean_up_node(&mut self, node: &'b VNode<'b>) { + for node in node.dynamic_nodes { + match node { + Component(comp) => { + let scope = comp.scope.get().unwrap(); + match unsafe { self.scopes[scope.0].root_node().extend_lifetime_ref() } { + RenderReturn::Sync(Ok(t)) => self.clean_up_node(t), + _ => todo!("cannot handle nonstandard nodes"), + }; + } + Text(t) => self.reclaim(t.id.get()), + Fragment(VFragment::Empty(t)) => self.reclaim(t.get()), + Fragment(VFragment::NonEmpty(nodes)) => { + nodes.into_iter().for_each(|node| self.clean_up_node(node)) + } + }; + } } fn remove_root_node(&mut self, node: &'b VNode<'b>, idx: usize) { let root = node.template.roots[idx]; match root { TemplateNode::Element { .. } | TemplateNode::Text(_) => { - self.mutations.push(Mutation::Remove { - id: node.root_ids[idx].get(), - }) + let id = node.root_ids[idx].get(); + self.mutations.push(Mutation::Remove { id }); + self.reclaim(id); } TemplateNode::DynamicText(id) | TemplateNode::Dynamic(id) => { match &node.dynamic_nodes[id] { - Text(i) => self.mutations.push(Mutation::Remove { id: i.id.get() }), + Text(i) => { + let id = i.id.get(); + self.mutations.push(Mutation::Remove { id }); + self.reclaim(id); + } Fragment(VFragment::Empty(e)) => { - self.mutations.push(Mutation::Remove { id: e.get() }) + let id = e.get(); + self.mutations.push(Mutation::Remove { id }); + self.reclaim(id); } Fragment(VFragment::NonEmpty(nodes)) => self.remove_nodes(nodes), Component(comp) => { @@ -789,26 +823,15 @@ impl<'b: 'static> VirtualDom { } } - fn replace_template(&mut self, left: &'b VNode<'b>, right: &'b VNode<'b>) { - todo!("replacing should work!") - } - - fn replace( - &mut self, - left_template: &'b VNode<'b>, - right_template: &'b VNode<'b>, - left: &'b DynamicNode<'b>, - right: &'b DynamicNode<'b>, - ) { - // Remove all but the first root - for root in &left_template.template.roots[1..] { - match root { - TemplateNode::Element { .. } => todo!(), - TemplateNode::Text(_) => todo!(), - TemplateNode::Dynamic(_) => todo!(), - TemplateNode::DynamicText(_) => todo!(), - } - } + fn replace(&mut self, left: &'b VNode<'b>, right: &'b VNode<'b>) { + let first = self.find_first_element(left); + let id = self.replace_inner(left); + let created = self.create(right); + self.mutations.push(Mutation::ReplaceWith { + id: first, + m: created, + }); + self.reclaim(id); } } diff --git a/packages/core/src/garbage.rs b/packages/core/src/garbage.rs index 056b14481..2d9e1d6d7 100644 --- a/packages/core/src/garbage.rs +++ b/packages/core/src/garbage.rs @@ -1,4 +1,6 @@ -use crate::{nodes::VNode, scopes::ScopeId, virtual_dom::VirtualDom, DynamicNode, Mutations}; +use crate::{ + nodes::VNode, scopes::ScopeId, virtual_dom::VirtualDom, DynamicNode, ElementId, Mutations, +}; impl<'b> VirtualDom { pub fn drop_scope(&mut self, id: ScopeId) { @@ -11,6 +13,11 @@ impl<'b> VirtualDom { todo!() } + pub fn reclaim(&mut self, el: ElementId) { + assert_ne!(el, ElementId(0)); + self.elements.remove(el.0); + } + pub fn drop_template( &mut self, mutations: &mut Mutations, diff --git a/packages/core/src/scheduler/wait.rs b/packages/core/src/scheduler/wait.rs index feb781b17..794abedc2 100644 --- a/packages/core/src/scheduler/wait.rs +++ b/packages/core/src/scheduler/wait.rs @@ -78,19 +78,22 @@ impl VirtualDom { let template: &VNode = unsafe { std::mem::transmute(template) }; let mutations: &mut Mutations = unsafe { std::mem::transmute(mutations) }; - todo!(); - // let place_holder_id = scope.placeholder.get().unwrap(); - // self.scope_stack.push(scope_id); - // let created = self.create(mutations, template); - // self.scope_stack.pop(); - // mutations.push(Mutation::ReplaceWith { - // id: place_holder_id, - // m: created, - // }); + std::mem::swap(&mut self.mutations, mutations); - // for leaf in self.collected_leaves.drain(..) { - // fiber.waiting_on.borrow_mut().insert(leaf); - // } + let place_holder_id = scope.placeholder.get().unwrap(); + self.scope_stack.push(scope_id); + let created = self.create(template); + self.scope_stack.pop(); + mutations.push(Mutation::ReplaceWith { + id: place_holder_id, + m: created, + }); + + for leaf in self.collected_leaves.drain(..) { + fiber.waiting_on.borrow_mut().insert(leaf); + } + + std::mem::swap(&mut self.mutations, mutations); if fiber.waiting_on.borrow().is_empty() { println!("fiber is finished!"); diff --git a/packages/core/tests/bubble_error.rs b/packages/core/tests/bubble_error.rs index 46a1a3579..310f5118d 100644 --- a/packages/core/tests/bubble_error.rs +++ b/packages/core/tests/bubble_error.rs @@ -1,69 +1,30 @@ //! we should properly bubble up errors from components -use std::{error::Error as StdError, marker::PhantomData, string::ParseError}; - -use anyhow::{anyhow, bail}; use dioxus::prelude::*; -// todo: add these to dioxus -pub trait Reject: Sized { - fn reject_err(self, t: impl FnOnce(E) -> anyhow::Error) -> Result { - todo!() - } - fn reject_because(self, t: impl Into) -> Result { - todo!() - } - - fn reject(self) -> Result { - todo!() - } -} - -impl Reject for &Result { - fn reject_err(self, t: impl FnOnce(E) -> anyhow::Error) -> Result { - todo!() - } -} - -/// Call "clone" on the underlying error so it can be propogated out -pub trait CloneErr { - fn clone_err(&self) -> Result<&T, E::Owned> - where - Self: Sized; -} - -impl CloneErr for Result { - fn clone_err(&self) -> Result<&T, E::Owned> - where - Self: Sized, - { - match self { - Ok(s) => Ok(s), - Err(e) => Err(e.to_owned()), - } - } -} - fn app(cx: Scope) -> Element { - // propgates error upwards, does not give a reason, lets Dioxus figure it out - let value = cx.use_hook(|| "123123123.123".parse::()).reject()?; + let raw = match cx.generation() % 2 { + 0 => "123.123", + 1 => "123.123.123", + _ => unreachable!(), + }; - // propgates error upwards, gives a reason - let value = cx - .use_hook(|| "123123123.123".parse::()) - .reject_because("Parsing float failed")?; + let value = raw.parse::()?; - let value = cx.use_hook(|| "123123123.123".parse::()).clone_err()?; - - let value = cx - .use_hook(|| "123123123.123".parse::()) - .as_ref() - .map_err(Clone::clone)?; - - let value = cx - .use_hook(|| "123123123.123".parse::()) - .as_ref() - .map_err(|_| anyhow!("Parsing float failed"))?; - - todo!() + cx.render(rsx! { + div { "hello {value}" } + }) +} + +#[test] +fn it_goes() { + let mut dom = VirtualDom::new(app); + + let edits = dom.rebuild().santize(); + + dbg!(edits); + + dom.mark_dirty_scope(ScopeId(0)); + + dom.render_immediate(); } diff --git a/packages/core/tests/create_fragments.rs b/packages/core/tests/create_fragments.rs index 6ec19372d..5b67711a8 100644 --- a/packages/core/tests/create_fragments.rs +++ b/packages/core/tests/create_fragments.rs @@ -16,7 +16,7 @@ fn empty_fragment_creates_nothing() { assert_eq!( edits.edits, [ - CreatePlaceholder { id: ElementId(2) }, + CreatePlaceholder { id: ElementId(1) }, AppendChildren { m: 1 } ] ); diff --git a/packages/core/tests/create_lists.rs b/packages/core/tests/create_lists.rs index 5848bc788..30d6abc2f 100644 --- a/packages/core/tests/create_lists.rs +++ b/packages/core/tests/create_lists.rs @@ -34,7 +34,7 @@ fn list_renders() { CreateElement { name: "div" }, // todo: since this is the only child, we should just use // append when modify the values (IE no need for a placeholder) - CreatePlaceholder { id: ElementId(0) }, + CreateStaticPlaceholder, AppendChildren { m: 1 }, SaveTemplate { name: "template", m: 1 }, // Create the inner template div diff --git a/packages/core/tests/cycle.rs b/packages/core/tests/cycle.rs new file mode 100644 index 000000000..803edede7 --- /dev/null +++ b/packages/core/tests/cycle.rs @@ -0,0 +1,51 @@ +use dioxus::core::{ElementId, Mutation::*}; +use dioxus::prelude::*; + +/// As we clean up old templates, the ID for the node should cycle +#[test] +fn cycling_elements() { + let mut dom = VirtualDom::new(|cx| { + cx.render(match cx.generation() % 2 { + 0 => rsx! { div { "wasd" } }, + 1 => rsx! { div { "abcd" } }, + _ => unreachable!(), + }) + }); + + let edits = dom.rebuild().santize(); + assert_eq!( + edits.edits, + [ + LoadTemplate { name: "template", index: 0, id: ElementId(1,) }, + AppendChildren { m: 1 }, + ] + ); + + dom.mark_dirty_scope(ScopeId(0)); + assert_eq!( + dom.render_immediate().santize().edits, + [ + LoadTemplate { name: "template", index: 0, id: ElementId(2,) }, + ReplaceWith { id: ElementId(1,), m: 1 }, + ] + ); + + // notice that the IDs cycle back to ElementId(1), preserving a minimal memory footprint + dom.mark_dirty_scope(ScopeId(0)); + assert_eq!( + dom.render_immediate().santize().edits, + [ + LoadTemplate { name: "template", index: 0, id: ElementId(1,) }, + ReplaceWith { id: ElementId(2,), m: 1 }, + ] + ); + + dom.mark_dirty_scope(ScopeId(0)); + assert_eq!( + dom.render_immediate().santize().edits, + [ + LoadTemplate { name: "template", index: 0, id: ElementId(2,) }, + ReplaceWith { id: ElementId(1,), m: 1 }, + ] + ); +} diff --git a/packages/core/tests/diff_element.rs b/packages/core/tests/diff_element.rs index 241adabe8..f4e5f108d 100644 --- a/packages/core/tests/diff_element.rs +++ b/packages/core/tests/diff_element.rs @@ -47,14 +47,38 @@ fn element_swap() { vdom.rebuild(); vdom.mark_dirty_scope(ScopeId(0)); - dbg!(vdom.render_immediate()); + assert_eq!( + vdom.render_immediate().santize().edits, + [ + LoadTemplate { name: "template", index: 0, id: ElementId(2,) }, + ReplaceWith { id: ElementId(1,), m: 1 }, + ] + ); vdom.mark_dirty_scope(ScopeId(0)); - dbg!(vdom.render_immediate()); + assert_eq!( + vdom.render_immediate().santize().edits, + [ + LoadTemplate { name: "template", index: 0, id: ElementId(3,) }, + ReplaceWith { id: ElementId(2,), m: 1 }, + ] + ); vdom.mark_dirty_scope(ScopeId(0)); - dbg!(vdom.render_immediate()); + assert_eq!( + vdom.render_immediate().santize().edits, + [ + LoadTemplate { name: "template", index: 0, id: ElementId(4,) }, + ReplaceWith { id: ElementId(3,), m: 1 }, + ] + ); vdom.mark_dirty_scope(ScopeId(0)); - dbg!(vdom.render_immediate()); + assert_eq!( + vdom.render_immediate().santize().edits, + [ + LoadTemplate { name: "template", index: 0, id: ElementId(5,) }, + ReplaceWith { id: ElementId(4,), m: 1 }, + ] + ); } diff --git a/packages/core/tests/suspend.rs b/packages/core/tests/suspend.rs index 13e4359d3..559c4e4ce 100644 --- a/packages/core/tests/suspend.rs +++ b/packages/core/tests/suspend.rs @@ -16,7 +16,7 @@ async fn it_works() { [ CreateElement { name: "div" }, CreateStaticText { value: "Waiting for child..." }, - CreatePlaceholder { id: ElementId(0) }, + CreateStaticPlaceholder, AppendChildren { m: 2 }, SaveTemplate { name: "template", m: 1 } ] diff --git a/packages/rsx/src/lib.rs b/packages/rsx/src/lib.rs index 15e9c00a9..f1fe0cafb 100644 --- a/packages/rsx/src/lib.rs +++ b/packages/rsx/src/lib.rs @@ -255,10 +255,9 @@ impl<'a> DynamicContext<'a> { self.dynamic_nodes.push(root); self.node_paths.push(self.current_path.clone()); - if let BodyNode::Text(_) = root { - quote! { ::dioxus::core::TemplateNode::DynamicText(#ct) } - } else { - quote! { ::dioxus::core::TemplateNode::Dynamic(#ct) } + match root { + BodyNode::Text(_) => quote! { ::dioxus::core::TemplateNode::DynamicText(#ct) }, + _ => quote! { ::dioxus::core::TemplateNode::Dynamic(#ct) }, } } }