From 478255f40d4de1d2e3f3cc9b6d758b30ff394b39 Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Mon, 11 Oct 2021 18:40:00 -0400 Subject: [PATCH] fix: all the bugs! --- packages/core/src/context.rs | 12 ++++-- packages/core/src/diff.rs | 54 +++++++++++--------------- packages/core/src/diff_stack.rs | 1 - packages/core/src/hooklist.rs | 16 ++++---- packages/core/src/resources.rs | 4 +- packages/core/src/scheduler.rs | 33 ++++++++++------ packages/core/src/scope.rs | 1 + packages/hooks/src/use_shared_state.rs | 17 ++++---- packages/web/src/lib.rs | 2 +- 9 files changed, 75 insertions(+), 65 deletions(-) diff --git a/packages/core/src/context.rs b/packages/core/src/context.rs index c0eca029f..833766e83 100644 --- a/packages/core/src/context.rs +++ b/packages/core/src/context.rs @@ -191,7 +191,7 @@ impl<'src> Context<'src> { /// rsx!(cx, div { "hello {state.0}" }) /// } /// ``` - pub fn provide_state(self, value: T) -> Option> + pub fn provide_state(self, value: T) where T: 'static, { @@ -200,7 +200,7 @@ impl<'src> Context<'src> { .borrow_mut() .insert(TypeId::of::(), Rc::new(value)) .map(|f| f.downcast::().ok()) - .flatten() + .flatten(); } /// Try to retrive a SharedState with type T from the any parent Scope. @@ -279,13 +279,17 @@ impl<'src> Context<'src> { Output: 'src, Init: FnOnce(usize) -> State, Run: FnOnce(&'src mut State) -> Output, - Cleanup: FnOnce(&mut State) + 'static, + Cleanup: FnOnce(Box) + 'static, { // If the idx is the same as the hook length, then we need to add the current hook if self.scope.hooks.at_end() { self.scope.hooks.push_hook( initializer(self.scope.hooks.len()), - Box::new(|raw| cleanup(raw.downcast_mut::().unwrap())), + Box::new(|raw| { + // + let s = raw.downcast::().unwrap(); + cleanup(s); + }), ); } diff --git a/packages/core/src/diff.rs b/packages/core/src/diff.rs index 062eb744d..99f8cb184 100644 --- a/packages/core/src/diff.rs +++ b/packages/core/src/diff.rs @@ -221,18 +221,12 @@ impl<'bump> DiffMachine<'bump> { MountType::Replace { old } => { if let Some(old_id) = old.try_mounted_id() { self.mutations.replace_with(old_id, nodes_created as u32); + self.remove_nodes(Some(old)); } else { - let mut iter = RealChildIterator::new(old, self.vdom); - let first = iter.next().unwrap(); - self.mutations - .replace_with(first.mounted_id(), nodes_created as u32); - self.remove_nodes(iter); - } - } - - MountType::ReplaceByElementId { el } => { - if let Some(old) = el { - self.mutations.replace_with(old, nodes_created as u32); + if let Some(id) = self.find_first_element_id(old) { + self.mutations.replace_with(id, nodes_created as u32); + } + self.remove_nodes(Some(old)); } } @@ -376,6 +370,8 @@ impl<'bump> DiffMachine<'bump> { let new_component = self.vdom.get_scope_mut(new_idx).unwrap(); + log::debug!("initializing component {:?}", new_idx); + // Run the scope for one iteration to initialize it if new_component.run_scope(self.vdom) { // Take the node that was just generated from running the component @@ -413,7 +409,7 @@ impl<'bump> DiffMachine<'bump> { (Fragment(old), Fragment(new)) => self.diff_fragment_nodes(old, new), (Anchor(old), Anchor(new)) => new.dom_id.set(old.dom_id.get()), (Suspended(old), Suspended(new)) => self.diff_suspended_nodes(old, new), - (Element(old), Element(new)) => self.diff_element_nodes(old, new, new_node), + (Element(old), Element(new)) => self.diff_element_nodes(old, new, old_node, new_node), // Anything else is just a basic replace and create ( @@ -439,6 +435,7 @@ impl<'bump> DiffMachine<'bump> { &mut self, old: &'bump VElement<'bump>, new: &'bump VElement<'bump>, + old_node: &'bump VNode<'bump>, new_node: &'bump VNode<'bump>, ) { let root = old.dom_id.get().unwrap(); @@ -452,9 +449,7 @@ impl<'bump> DiffMachine<'bump> { // issue is that we need the "vnode" but this method only has the velement self.stack.push_nodes_created(0); self.stack.push(DiffInstruction::Mount { - and: MountType::ReplaceByElementId { - el: old.dom_id.get(), - }, + and: MountType::Replace { old: old_node }, }); self.create_element_node(new, new_node); return; @@ -546,6 +541,7 @@ impl<'bump> DiffMachine<'bump> { // Make sure we're dealing with the same component (by function pointer) if old.user_fc == new.user_fc { + log::debug!("Diffing component {:?} - {:?}", new.user_fc, scope_addr); // self.stack.scope_stack.push(scope_addr); @@ -554,7 +550,6 @@ impl<'bump> DiffMachine<'bump> { // make sure the component's caller function is up to date let scope = self.vdom.get_scope_mut(scope_addr).unwrap(); - scope.update_scope_dependencies(new.caller, ScopeChildren(new.children)); // React doesn't automatically memoize, but we do. @@ -569,8 +564,6 @@ impl<'bump> DiffMachine<'bump> { } self.stack.scope_stack.pop(); - - self.seen_scopes.insert(scope_addr); } else { self.stack .create_node(new_node, MountType::Replace { old: old_node }); @@ -624,17 +617,14 @@ impl<'bump> DiffMachine<'bump> { self.stack.create_children(new, MountType::Append); } (_, []) => { - for node in old { - self.remove_nodes(Some(node)); - } + self.remove_nodes(old); } ([VNode::Anchor(old_anchor)], [VNode::Anchor(new_anchor)]) => { old_anchor.dom_id.set(new_anchor.dom_id.get()); } - ([VNode::Anchor(anchor)], _) => { - let el = anchor.dom_id.get(); + ([VNode::Anchor(_)], _) => { self.stack - .create_children(new, MountType::ReplaceByElementId { el }); + .create_children(new, MountType::Replace { old: &old[0] }); } (_, [VNode::Anchor(_)]) => { self.replace_and_create_many_with_one(old, &new[0]); @@ -743,13 +733,13 @@ impl<'bump> DiffMachine<'bump> { Some(count) => count, None => return, }; - log::debug!( - "Left offset, right offset, {}, {}", - left_offset, - right_offset, - ); + // log::debug!( + // "Left offset, right offset, {}, {}", + // left_offset, + // right_offset, + // ); - log::debug!("stack before lo is {:#?}", self.stack.instructions); + // log::debug!("stack before lo is {:#?}", self.stack.instructions); // Ok, we now hopefully have a smaller range of children in the middle // within which to re-order nodes with the same keys, remove old nodes with // now-unused keys, and create new nodes with fresh keys. @@ -1126,9 +1116,11 @@ impl<'bump> DiffMachine<'bump> { VNode::Component(c) => { let scope_id = c.associated_scope.get().unwrap(); - let scope = self.vdom.get_scope(scope_id).unwrap(); + let scope = self.vdom.get_scope_mut(scope_id).unwrap(); let root = scope.root_node(); self.remove_nodes(Some(root)); + let mut s = self.vdom.try_remove(scope_id).unwrap(); + s.hooks.clear_hooks(); } } } diff --git a/packages/core/src/diff_stack.rs b/packages/core/src/diff_stack.rs index 480712df2..fc1ec9335 100644 --- a/packages/core/src/diff_stack.rs +++ b/packages/core/src/diff_stack.rs @@ -30,7 +30,6 @@ pub enum MountType<'a> { Absorb, Append, Replace { old: &'a VNode<'a> }, - ReplaceByElementId { el: Option }, InsertAfter { other_node: &'a VNode<'a> }, InsertBefore { other_node: &'a VNode<'a> }, } diff --git a/packages/core/src/hooklist.rs b/packages/core/src/hooklist.rs index 2f68be3b8..cb3e1637d 100644 --- a/packages/core/src/hooklist.rs +++ b/packages/core/src/hooklist.rs @@ -12,7 +12,7 @@ use std::{ /// Todo: this could use its very own bump arena, but that might be a tad overkill #[derive(Default)] pub(crate) struct HookList { - vals: RefCell>, Box)>>, + vals: RefCell>, Box)>)>>, idx: Cell, } @@ -35,7 +35,7 @@ impl HookList { self.idx.set(0); } - pub(crate) fn push_hook(&self, new: T, cleanup: Box) { + pub(crate) fn push_hook(&self, new: T, cleanup: Box)>) { self.vals .borrow_mut() .push((UnsafeCell::new(Box::new(new)), cleanup)) @@ -52,14 +52,16 @@ impl HookList { pub(crate) fn at_end(&self) -> bool { self.cur_idx() >= self.len() } -} -// When the scope is dropped, we want to call the cleanup function for each of the hooks -impl Drop for HookList { - fn drop(&mut self) { + pub fn clear_hooks(&mut self) { self.vals .borrow_mut() .drain(..) - .for_each(|(mut state, cleanup)| cleanup(state.get_mut())); + .for_each(|(mut state, cleanup)| { + // + let s: Box = state.into_inner(); + cleanup(s) + // + }); } } diff --git a/packages/core/src/resources.rs b/packages/core/src/resources.rs index ebdd6ef49..f0179cb1d 100644 --- a/packages/core/src/resources.rs +++ b/packages/core/src/resources.rs @@ -63,8 +63,8 @@ impl ResourcePool { /// return the id, freeing the space of the original node pub fn collect_garbage(&self, id: ElementId) { - todo!("garabge collection currently WIP") - // self.raw_elements.remove(id.0); + let els = unsafe { &mut *self.raw_elements.get() }; + els.remove(id.0); } pub fn insert_scope_with_key(&self, f: impl FnOnce(ScopeId) -> Scope) -> ScopeId { diff --git a/packages/core/src/scheduler.rs b/packages/core/src/scheduler.rs index 3f2c09a15..20005a33e 100644 --- a/packages/core/src/scheduler.rs +++ b/packages/core/src/scheduler.rs @@ -185,7 +185,11 @@ impl Scheduler { sender: sender.clone(), schedule_any_immediate: { let sender = sender.clone(); - Rc::new(move |id| sender.unbounded_send(SchedulerMsg::Immediate(id)).unwrap()) + Rc::new(move |id| { + // + log::debug!("scheduling immedate! {:?}", id); + sender.unbounded_send(SchedulerMsg::Immediate(id)).unwrap() + }) }, // todo: we want to get the futures out of the scheduler message // the scheduler message should be send/sync @@ -374,23 +378,28 @@ impl Scheduler { if machine.stack.is_empty() { let shared = self.pool.clone(); + self.dirty_scopes + .retain(|id| shared.get_scope(*id).is_some()); self.dirty_scopes.sort_by(|a, b| { let h1 = shared.get_scope(*a).unwrap().height; let h2 = shared.get_scope(*b).unwrap().height; - h1.cmp(&h2) + h1.cmp(&h2).reverse() }); if let Some(scopeid) = self.dirty_scopes.pop() { - log::info!("handlng dirty scope {:#?}", scopeid); + log::info!("handlng dirty scope {:?}", scopeid); if !ran_scopes.contains(&scopeid) { ran_scopes.insert(scopeid); + log::debug!("about to run scope {:?}", scopeid); - let mut component = self.pool.get_scope_mut(scopeid).unwrap(); - if component.run_scope(&self.pool) { - let (old, new) = (component.frames.wip_head(), component.frames.fin_head()); - // let (old, new) = (component.frames.wip_head(), component.frames.fin_head()); - machine.stack.scope_stack.push(scopeid); - machine.stack.push(DiffInstruction::Diff { new, old }); + if let Some(component) = self.pool.get_scope_mut(scopeid) { + if component.run_scope(&self.pool) { + let (old, new) = + (component.frames.wip_head(), component.frames.fin_head()); + // let (old, new) = (component.frames.wip_head(), component.frames.fin_head()); + machine.stack.scope_stack.push(scopeid); + machine.stack.push(DiffInstruction::Diff { new, old }); + } } } } @@ -522,8 +531,6 @@ impl Scheduler { } } - log::debug!("work with deadline completed: {:#?}", committed_mutations); - committed_mutations } @@ -562,6 +569,8 @@ impl Scheduler { .get_scope_mut(base_scope) .expect("The base scope should never be moved"); + log::debug!("rebuild {:?}", base_scope); + // We run the component. If it succeeds, then we can diff it and add the changes to the dom. if cur_component.run_scope(&self.pool) { diff_machine @@ -588,6 +597,8 @@ impl Scheduler { .get_scope_mut(base_scope) .expect("The base scope should never be moved"); + log::debug!("hard diff {:?}", base_scope); + if cur_component.run_scope(&self.pool) { let mut diff_machine = DiffMachine::new(Mutations::new(), &mut self.pool); diff_machine.cfg.force_diff = true; diff --git a/packages/core/src/scope.rs b/packages/core/src/scope.rs index c1d5981fa..b4cbb6e5d 100644 --- a/packages/core/src/scope.rs +++ b/packages/core/src/scope.rs @@ -218,6 +218,7 @@ impl Scope { caller: &'creator_node dyn for<'b> Fn(&'b Scope) -> DomTree<'b>, child_nodes: ScopeChildren, ) { + log::debug!("Updating scope dependencies {:?}", self.our_arena_idx); let caller = caller as *const _; self.caller = unsafe { std::mem::transmute(caller) }; diff --git a/packages/hooks/src/use_shared_state.rs b/packages/hooks/src/use_shared_state.rs index e2a8f5399..b1febb498 100644 --- a/packages/hooks/src/use_shared_state.rs +++ b/packages/hooks/src/use_shared_state.rs @@ -112,10 +112,10 @@ impl<'a, T: 'static> UseSharedState<'a, T> { } pub fn notify_consumers(self) { - if !self.needs_notification.get() { - self.needs_notification.set(true); - self.root.borrow_mut().notify_consumers(); - } + // if !self.needs_notification.get() { + self.root.borrow_mut().notify_consumers(); + // self.needs_notification.set(true); + // } } pub fn read_write(&self) -> (Ref<'_, T>, &Self) { @@ -161,16 +161,17 @@ where /// /// /// -pub fn use_provide_state<'a, T: 'static>(cx: Context<'a>, f: impl FnOnce() -> T) -> Option<()> { +pub fn use_provide_state<'a, T: 'static>(cx: Context<'a>, f: impl FnOnce() -> T) { cx.use_hook( |_| { - cx.provide_state(ProvidedStateInner { + let state: ProvidedState = RefCell::new(ProvidedStateInner { value: Rc::new(RefCell::new(f())), notify_any: cx.schedule_update_any(), consumers: HashSet::new(), - }) + }); + cx.provide_state(state) }, - |inner| inner.as_ref().and_then(|_| Some(())), + |inner| {}, |_| {}, ) } diff --git a/packages/web/src/lib.rs b/packages/web/src/lib.rs index f613692cc..368023f37 100644 --- a/packages/web/src/lib.rs +++ b/packages/web/src/lib.rs @@ -176,7 +176,7 @@ pub async fn run_with_props(root: FC, root_props: T, cfg: work_loop.wait_for_raf().await; for mut edit in mutations { - log::debug!("edits are {:#?}", edit); + // log::debug!("edits are {:#?}", edit); // actually apply our changes during the animation frame websys_dom.process_edits(&mut edit.edits); }