Browse Source

Fix diffing cloned rsx nodes by deep cloning nodes before mounting them (#3271)

* Fix diffing cloned rsx nodes by deep cloning nodes before mounting them

* fix clippy
Evan Almloff 6 months ago
parent
commit
354c4ce9cc

+ 8 - 0
packages/core/src/error_boundary.rs

@@ -459,6 +459,14 @@ impl CapturedError {
             Some(std::result::Result::Ok(self.render.clone()))
         }
     }
+
+    /// Create a deep clone of this error
+    pub(crate) fn deep_clone(&self) -> Self {
+        Self {
+            render: self.render.deep_clone(),
+            ..self.clone()
+        }
+    }
 }
 
 impl PartialEq for CapturedError {

+ 10 - 2
packages/core/src/events.rs

@@ -425,8 +425,8 @@ impl<Args, Ret> Clone for Callback<Args, Ret> {
 }
 
 impl<Args: 'static, Ret: 'static> PartialEq for Callback<Args, Ret> {
-    fn eq(&self, _: &Self) -> bool {
-        true
+    fn eq(&self, other: &Self) -> bool {
+        self.callback.ptr_eq(&other.callback) && self.origin == other.origin
     }
 }
 
@@ -471,6 +471,14 @@ impl<Args: 'static, Ret: 'static> Callback<Args, Ret> {
         Self { callback, origin }
     }
 
+    /// Leak a new reference to the [`Callback`] that will not be dropped unless the callback is dropped manually
+    pub(crate) fn leak_reference(&self) -> generational_box::BorrowResult<Callback<Args, Ret>> {
+        Ok(Callback {
+            callback: self.callback.leak_reference()?,
+            origin: self.origin,
+        })
+    }
+
     /// Call this callback with the appropriate argument type
     ///
     /// This borrows the callback using a RefCell. Recursively calling a callback will cause a panic.

+ 59 - 1
packages/core/src/nodes.rs

@@ -132,12 +132,23 @@ impl From<Element> for VNode {
 pub(crate) trait AsVNode {
     /// Get the vnode for this element
     fn as_vnode(&self) -> &VNode;
+
+    /// Create a deep clone of this VNode
+    fn deep_clone(&self) -> Self;
 }
 
 impl AsVNode for Element {
     fn as_vnode(&self) -> &VNode {
         AsRef::as_ref(self)
     }
+
+    fn deep_clone(&self) -> Self {
+        match self {
+            Ok(node) => Ok(node.deep_clone()),
+            Err(RenderError::Aborted(err)) => Err(RenderError::Aborted(err.deep_clone())),
+            Err(RenderError::Suspended(fut)) => Err(RenderError::Suspended(fut.deep_clone())),
+        }
+    }
 }
 
 impl Default for VNode {
@@ -288,6 +299,38 @@ impl VNode {
             .get(dynamic_attribute_idx)
             .copied()
     }
+
+    /// Create a deep clone of this VNode
+    pub(crate) fn deep_clone(&self) -> Self {
+        Self {
+            vnode: Rc::new(VNodeInner {
+                key: self.vnode.key.clone(),
+                template: self.vnode.template,
+                dynamic_nodes: self
+                    .vnode
+                    .dynamic_nodes
+                    .iter()
+                    .map(|node| match node {
+                        DynamicNode::Fragment(nodes) => DynamicNode::Fragment(
+                            nodes.iter().map(|node| node.deep_clone()).collect(),
+                        ),
+                        other => other.clone(),
+                    })
+                    .collect(),
+                dynamic_attrs: self
+                    .vnode
+                    .dynamic_attrs
+                    .iter()
+                    .map(|attr| {
+                        attr.iter()
+                            .map(|attribute| attribute.deep_clone())
+                            .collect()
+                    })
+                    .collect(),
+            }),
+            mount: Default::default(),
+        }
+    }
 }
 
 type StaticStr = &'static str;
@@ -737,6 +780,21 @@ impl Attribute {
             value: value.into_value(),
         }
     }
+
+    /// Create a new deep clone of this attribute
+    pub(crate) fn deep_clone(&self) -> Self {
+        Attribute {
+            name: self.name,
+            namespace: self.namespace,
+            volatile: self.volatile,
+            value: match &self.value {
+                AttributeValue::Listener(listener) => {
+                    AttributeValue::Listener(listener.leak_reference().unwrap())
+                }
+                value => value.clone(),
+            },
+        }
+    }
 }
 
 /// Any of the built-in values that the Dioxus VirtualDom supports as dynamic attributes on elements
@@ -812,7 +870,7 @@ impl PartialEq for AttributeValue {
             (Self::Float(l0), Self::Float(r0)) => l0 == r0,
             (Self::Int(l0), Self::Int(r0)) => l0 == r0,
             (Self::Bool(l0), Self::Bool(r0)) => l0 == r0,
-            (Self::Listener(_), Self::Listener(_)) => true,
+            (Self::Listener(l0), Self::Listener(r0)) => l0 == r0,
             (Self::Any(l0), Self::Any(r0)) => l0.as_ref().any_cmp(r0.as_ref()),
             (Self::None, Self::None) => true,
             _ => false,

+ 13 - 1
packages/core/src/scope_arena.rs

@@ -1,4 +1,5 @@
 use crate::innerlude::{throw_error, RenderError, ScopeOrder};
+use crate::nodes::AsVNode;
 use crate::prelude::ReactiveContext;
 use crate::scope_context::SuspenseLocation;
 use crate::{
@@ -69,7 +70,18 @@ impl VirtualDom {
                 let span = tracing::trace_span!("render", scope = %scope.state().name);
                 span.in_scope(|| {
                     scope.reactive_context.reset_and_run_in(|| {
-                        let mut render_return = props.render();
+                        let render_return = props.render();
+                        // After the component is run, we need to do a deep clone of the VNode. This
+                        // breaks any references to mounted parts of the VNode from the component.
+                        // Without this, the component could store a mounted version of the VNode
+                        // which causes a lot of issues for diffing because we expect only the old
+                        // or new node to be mounted.
+                        //
+                        // For example, the dog app example returns rsx from a resource. Every time
+                        // the component runs, it returns a clone of the last rsx that was returned from
+                        // that resource. If we don't deep clone the VNode and the resource changes, then
+                        // we could end up diffing two different versions of the same mounted node
+                        let mut render_return = render_return.deep_clone();
                         self.handle_element_return(&mut render_return, scope_id, &scope.state());
                         render_return
                     })

+ 9 - 0
packages/core/src/suspense/mod.rs

@@ -70,6 +70,15 @@ impl SuspendedFuture {
     pub fn task(&self) -> Task {
         self.task
     }
+
+    /// Create a deep clone of this suspended future
+    pub(crate) fn deep_clone(&self) -> Self {
+        Self {
+            task: self.task,
+            placeholder: self.placeholder.deep_clone(),
+            origin: self.origin,
+        }
+    }
 }
 
 /// A context with information about suspended components