From 00231adfa2e1d67a9d7ae2fa61c33e3a22d51978 Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Thu, 15 Jul 2021 04:09:28 -0400 Subject: [PATCH] feat: code quality improvements for core --- packages/core/src/arena.rs | 11 +- packages/core/src/context.rs | 28 +- packages/core/src/debug_renderer.rs | 32 -- packages/core/src/editor.rs | 2 +- packages/core/src/lib.rs | 3 + packages/core/src/nodes.rs | 126 +++++--- packages/core/src/oldbuilder.rs | 484 ---------------------------- packages/core/src/scope.rs | 61 ++-- packages/core/src/signals.rs | 5 +- packages/core/src/tasks.rs | 42 --- packages/core/src/util.rs | 80 +---- packages/ssr/src/lib.rs | 8 +- 12 files changed, 124 insertions(+), 758 deletions(-) delete mode 100644 packages/core/src/oldbuilder.rs diff --git a/packages/core/src/arena.rs b/packages/core/src/arena.rs index 7720d8256..7f3b47f3e 100644 --- a/packages/core/src/arena.rs +++ b/packages/core/src/arena.rs @@ -1,16 +1,11 @@ -use std::{ - cell::{RefCell, UnsafeCell}, - collections::HashMap, - rc::Rc, - sync::Arc, -}; +use std::{cell::UnsafeCell, rc::Rc}; use crate::innerlude::*; use slotmap::{DefaultKey, SlotMap}; #[derive(Clone)] pub struct SharedArena { - pub components: Arc>, + pub components: Rc>, } pub type ScopeMap = SlotMap; @@ -21,7 +16,7 @@ enum MutStatus { impl SharedArena { pub fn new(arena: ScopeMap) -> Self { - let components = Arc::new(UnsafeCell::new(arena)); + let components = Rc::new(UnsafeCell::new(arena)); SharedArena { components } } diff --git a/packages/core/src/context.rs b/packages/core/src/context.rs index faa1df831..dbef3052a 100644 --- a/packages/core/src/context.rs +++ b/packages/core/src/context.rs @@ -1,15 +1,11 @@ use crate::innerlude::*; use futures_util::FutureExt; - -use std::any::Any; -use std::cell::Cell; -use std::marker::PhantomData; - use std::{ - any::TypeId, - cell::RefCell, + any::{Any, TypeId}, + cell::{Cell, RefCell}, future::Future, + marker::PhantomData, ops::Deref, rc::{Rc, Weak}, }; @@ -316,23 +312,7 @@ Any function prefixed with "use" should not be called conditionally. /// /// pub fn submit_task(&self, task: FiberTask) -> TaskHandle { - let r = (self.scope.task_submitter)(task); - // self.scope.submit_task(task); - // let r = task.then(|f| async { - // // - // }); - // self.use_hook(|| Box::new(r), |_| {}, |_| {}); - // *task = task.then(|f| async { - // // - // t - // // () - // }); - // let new_fut = task.then(|f| async { - // // - // () - // }); - // self.tasks.borrow_mut().push(new_fut); - + (self.scope.task_submitter)(task); TaskHandle { _p: PhantomData {} } } diff --git a/packages/core/src/debug_renderer.rs b/packages/core/src/debug_renderer.rs index b565f68ba..38604cb1c 100644 --- a/packages/core/src/debug_renderer.rs +++ b/packages/core/src/debug_renderer.rs @@ -76,35 +76,3 @@ impl DebugRenderer { Ok(()) } } - -pub struct DebugVNodeSource { - bump: Bump, -} -impl DebugVNodeSource { - fn new() -> Self { - Self { bump: Bump::new() } - } - - fn render_nodes(&self) -> VNode { - // let cx = NodeFactory - todo!() - } -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn ensure_creation() -> Result<(), ()> { - // static Example: FC<()> = |cx| { - // // - // cx.render(html! {
"hello world"
}) - // }; - - // let mut dom = VirtualDom::new(Example); - // let machine = DiffMachine::new(); - - Ok(()) - } -} diff --git a/packages/core/src/editor.rs b/packages/core/src/editor.rs index 3c227c4fe..c443f884c 100644 --- a/packages/core/src/editor.rs +++ b/packages/core/src/editor.rs @@ -12,7 +12,7 @@ use crate::{innerlude::ScopeId, RealDomNode}; /// However, the DomEditor only builds a change list - it does not apply them. In contrast with the "RealDom", the DomEditor /// is cancellable and flushable. At any moment in time, Dioxus may choose to completely clear the edit list and start over. /// -/// This behavior is used in the cooperative scheduling algorithm +/// This behavior is used in the cooperative scheduling algorithm. pub struct DomEditor<'real, 'bump> { pub edits: &'real mut Vec>, } diff --git a/packages/core/src/lib.rs b/packages/core/src/lib.rs index d9241e519..e749cf673 100644 --- a/packages/core/src/lib.rs +++ b/packages/core/src/lib.rs @@ -26,6 +26,7 @@ pub mod prelude { // types used internally that are important pub(crate) mod innerlude { + pub use crate::arena::*; pub use crate::bumpframe::*; pub use crate::component::*; pub use crate::context::*; @@ -33,6 +34,7 @@ pub(crate) mod innerlude { pub use crate::editor::*; pub use crate::error::*; pub use crate::events::*; + pub use crate::hooklist::*; pub use crate::nodes::*; pub use crate::scope::*; pub use crate::tasks::*; @@ -46,6 +48,7 @@ pub(crate) mod innerlude { pub mod exports { // export important things here + pub use bumpalo; } pub mod arena; diff --git a/packages/core/src/nodes.rs b/packages/core/src/nodes.rs index b781ad17d..f29425d5c 100644 --- a/packages/core/src/nodes.rs +++ b/packages/core/src/nodes.rs @@ -1,8 +1,8 @@ //! Virtual Node Support +//! -------------------- //! VNodes represent lazily-constructed VDom trees that support diffing and event handlers. //! //! These VNodes should be *very* cheap and *very* fast to construct - building a full tree should be insanely quick. - use crate::{ events::VirtualEvent, innerlude::{Context, Properties, RealDomNode, Scope, ScopeId, FC}, @@ -16,8 +16,13 @@ use std::{ pub struct VNode<'src> { pub kind: VNodeKind<'src>, - pub dom_id: Cell, - pub key: Option<&'src str>, + pub(crate) dom_id: Cell, + pub(crate) key: Option<&'src str>, +} +impl VNode<'_> { + fn key(&self) -> Option<&str> { + self.key + } } /// Tools for the base unit of the virtual dom - the VNode @@ -27,9 +32,13 @@ pub struct VNode<'src> { /// limit the amount of heap allocations / overly large enum sizes. pub enum VNodeKind<'src> { Text(VText<'src>), + Element(&'src VElement<'src>), + Fragment(VFragment<'src>), + Component(&'src VComponent<'src>), + Suspended { node: Rc> }, } @@ -55,6 +64,7 @@ pub trait DioxusElement { Self::NAME_SPACE } } + pub struct VElement<'a> { // tag is always static pub tag_name: &'static str, @@ -75,9 +85,13 @@ pub struct VElement<'a> { #[derive(Clone, Debug)] pub struct Attribute<'a> { pub name: &'static str, + pub value: &'a str, + pub is_static: bool, + pub is_volatile: bool, + // Doesn't exist in the html spec, mostly used to denote "style" tags - could be for any type of group pub namespace: Option<&'static str>, } @@ -87,8 +101,11 @@ pub struct Attribute<'a> { pub struct Listener<'bump> { /// The type of event to listen for. pub(crate) event: &'static str, + pub scope: ScopeId, - pub mounted_node: &'bump Cell, + + pub mounted_node: &'bump mut Cell, + pub(crate) callback: &'bump dyn FnMut(VirtualEvent), } @@ -96,9 +113,13 @@ pub struct Listener<'bump> { /// Only supports the functional syntax pub struct VComponent<'src> { pub ass_scope: Cell>, + pub(crate) caller: Rc VNode>, + pub(crate) children: &'src [VNode<'src>], + pub(crate) comparator: Option<&'src dyn Fn(&VComponent) -> bool>, + pub is_static: bool, // a pointer into the bump arena (given by the 'src lifetime) @@ -124,20 +145,7 @@ impl<'a> NodeFactory<'a> { &self.scope_ref.cur_frame().bump } - pub const fn const_text(&self, text: &'static str) -> VNodeKind<'static> { - VNodeKind::Text(VText { - is_static: true, - text, - }) - } - - pub const fn const_fragment(&self, children: &'static [VNode<'static>]) -> VNodeKind<'static> { - VNodeKind::Fragment(VFragment { - children, - is_static: true, - }) - } - + /// Used in a place or two to make it easier to build vnodes from dummy text pub fn static_text(text: &'static str) -> VNode { VNode { dom_id: RealDomNode::empty_cell(), @@ -149,6 +157,9 @@ impl<'a> NodeFactory<'a> { } } + /// Parses a lazy text Arguments and returns a string and a flag indicating if the text is 'static + /// + /// Text that's static may be pointer compared, making it cheaper to diff pub fn raw_text(&self, args: Arguments) -> (&'a str, bool) { match args.as_str() { Some(static_str) => (static_str, true), @@ -162,6 +173,7 @@ impl<'a> NodeFactory<'a> { } /// Create some text that's allocated along with the other vnodes + /// pub fn text(&self, args: Arguments) -> VNode<'a> { let (text, is_static) = self.raw_text(args); VNode { @@ -171,27 +183,40 @@ impl<'a> NodeFactory<'a> { } } - pub fn raw_element( - &self, - _tag: &'static str, - _listeners: &[Listener], - _attributes: &[Attribute], - _children: &'a [VNode<'a>], - ) -> VNode<'a> { - todo!() - } - pub fn element( &self, el: impl DioxusElement, - listeners: &'a [Listener<'a>], + listeners: &'a mut [Listener<'a>], attributes: &'a [Attribute<'a>], children: &'a [VNode<'a>], key: Option<&'a str>, ) -> VNode<'a> { + self.raw_element( + el.tag_name(), + el.namespace(), + listeners, + attributes, + children, + key, + ) + } + + pub fn raw_element( + &self, + tag: &'static str, + namespace: Option<&'static str>, + listeners: &'a mut [Listener], + attributes: &'a [Attribute], + children: &'a [VNode<'a>], + key: Option<&'a str>, + ) -> VNode<'a> { + // We take the references directly from the bump arena + // TODO: this code shouldn't necessarily be here of all places + // It would make more sense to do this in diffing + let mut queue = self.scope_ref.listeners.borrow_mut(); - for listener in listeners { - let mounted = listener.mounted_node as *const _ as *mut _; + for listener in listeners.iter_mut() { + let mounted = listener.mounted_node as *mut _; let callback = listener.callback as *const _ as *mut _; queue.push((mounted, callback)) } @@ -200,14 +225,16 @@ impl<'a> NodeFactory<'a> { dom_id: RealDomNode::empty_cell(), key, kind: VNodeKind::Element(self.bump().alloc(VElement { - tag_name: el.tag_name(), - namespace: el.namespace(), - static_listeners: false, + tag_name: tag, + namespace, listeners, - static_attrs: false, attributes, - static_children: false, children, + + // todo: wire up more constization + static_listeners: false, + static_attrs: false, + static_children: false, })), } } @@ -243,12 +270,15 @@ impl<'a> NodeFactory<'a> { &self, component: FC

, props: P, - key: Option<&'a str>, // key: NodeKey<'a>, + key: Option<&'a str>, children: &'a [VNode<'a>], ) -> VNode<'a> where P: Properties + 'a, { + // TODO + // It's somewhat wrong to go about props like this + // We don't want the fat part of the fat pointer // This function does static dispatch so we don't need any VTable stuff let props = self.bump().alloc(props); @@ -271,6 +301,8 @@ impl<'a> NodeFactory<'a> { } })); + let is_static = children.len() == 0 && P::IS_STATIC && key.is_none(); + VNode { key, dom_id: Cell::new(RealDomNode::empty()), @@ -280,7 +312,7 @@ impl<'a> NodeFactory<'a> { raw_props, children, caller: NodeFactory::create_component_caller(component, raw_props), - is_static: children.len() == 0 && P::IS_STATIC && key.is_none(), + is_static, ass_scope: Cell::new(None), })), } @@ -313,10 +345,26 @@ impl<'a> NodeFactory<'a> { node_iter: impl IntoIterator>, ) -> VNode<'a> { let mut nodes = bumpalo::collections::Vec::new_in(self.bump()); - // TODO throw an error if there are nodes without keys + for node in node_iter.into_iter() { nodes.push(node.into_vnode(self)); } + + if cfg!(debug_assertions) { + if nodes.len() > 1 { + if nodes.last().unwrap().key().is_none() { + log::error!( + r#" +Warning: Each child in an array or iterator should have a unique "key" prop. +Not providing a key will lead to poor performance with lists. +See docs.rs/dioxus for more information. +--- +To help you identify where this error is coming from, we've generated a backtrace. + "#, + ); + } + } + } VNode { dom_id: RealDomNode::empty_cell(), key: None, diff --git a/packages/core/src/oldbuilder.rs b/packages/core/src/oldbuilder.rs deleted file mode 100644 index c3d52b0c0..000000000 --- a/packages/core/src/oldbuilder.rs +++ /dev/null @@ -1,484 +0,0 @@ -/// Typically constructed with element-specific constructors, eg the `div` -/// function for building `

` elements or the `button` function for building -/// `