Fix issue with node refs and hydration (#2597)

* Fix issue with node refs and hydration

When a component is contained in a component, the
inner reconciles, which used to replace the NodeRef,
which left a badly linked one in the outer Hydration render state.

Now, keep a stable internal_ref besides the user-passed node_ref.
The internal_ref never gets replaced as long as the BComp lives.

packages/yew/src/dom_bundle/bcomp.rs

@@ -12,6 +12,11 @@ use web_sys::Element;
 pub(super) struct BComp {
     type_id: TypeId,
     scope: Box<dyn Scoped>,
+    // A internal NodeRef passed around to track this components position. This
+    // is "stable", i.e. does not change when reconciled.
+    internal_ref: NodeRef,
+    // The user-passed NodeRef from VComp. Might change every time we reconcile.
+    // Gets linked to the internal ref
     node_ref: NodeRef,
     key: Option<Key>,
 }
@@ -57,20 +62,23 @@ impl Reconcilable for VComp {
             node_ref,
             key,
         } = self;
+        let internal_ref = NodeRef::default();
+        node_ref.link(internal_ref.clone());
 
         let scope = mountable.mount(
             root,
-            node_ref.clone(),
+            internal_ref.clone(),
             parent_scope,
             parent.to_owned(),
             next_sibling,
         );
 
         (
-            node_ref.clone(),
+            internal_ref.clone(),
             BComp {
                 type_id,
                 node_ref,
+                internal_ref,
                 key,
                 scope,
             },
@@ -112,10 +120,10 @@ impl Reconcilable for VComp {
         } = self;
 
         bcomp.key = key;
-        let old_ref = std::mem::replace(&mut bcomp.node_ref, node_ref.clone());
+        let old_ref = std::mem::replace(&mut bcomp.node_ref, node_ref);
         bcomp.node_ref.reuse(old_ref);
-        mountable.reuse(node_ref.clone(), bcomp.scope.borrow(), next_sibling);
+        mountable.reuse(bcomp.scope.borrow(), next_sibling);
-        node_ref
+        bcomp.internal_ref.clone()
     }
 }
 
@@ -139,21 +147,24 @@ mod feat_hydration {
                 node_ref,
                 key,
             } = self;
+            let internal_ref = NodeRef::default();
+            node_ref.link(internal_ref.clone());
 
             let scoped = mountable.hydrate(
                 root.clone(),
                 parent_scope,
                 parent.clone(),
                 fragment,
-                node_ref.clone(),
+                internal_ref.clone(),
             );
 
             (
-                node_ref.clone(),
+                internal_ref.clone(),
                 BComp {
                     type_id,
                     scope: scoped,
                     node_ref,
+                    internal_ref,
                     key,
                 },
             )
@@ -165,7 +176,7 @@ mod feat_hydration {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::dom_bundle::{Reconcilable, ReconcileTarget};
+    use crate::dom_bundle::{Bundle, Reconcilable, ReconcileTarget};
     use crate::scheduler;
     use crate::{
         html,
@@ -442,6 +453,33 @@ mod tests {
         scheduler::start_now();
         assert!(node_ref.get().is_none());
     }
+
+    #[test]
+    fn reset_ancestors_node_ref() {
+        let (root, scope, parent) = setup_parent();
+
+        let mut bundle = Bundle::new();
+        let node_ref_a = NodeRef::default();
+        let node_ref_b = NodeRef::default();
+        let elem = html! { <Comp ref={node_ref_a.clone()}></Comp> };
+        let node_a = bundle.reconcile(&root, &scope, &parent, NodeRef::default(), elem);
+        scheduler::start_now();
+        let node_a = node_a.get().unwrap();
+
+        assert!(node_ref_a.get().is_some(), "node_ref_a should be bound");
+
+        let elem = html! { <Comp ref={node_ref_b.clone()}></Comp> };
+        let node_b = bundle.reconcile(&root, &scope, &parent, NodeRef::default(), elem);
+        scheduler::start_now();
+        let node_b = node_b.get().unwrap();
+
+        assert_eq!(node_a, node_b, "Comp should have reused the element");
+        assert!(node_ref_b.get().is_some(), "node_ref_b should be bound");
+        assert!(
+            node_ref_a.get().is_none(),
+            "node_ref_a should have been reset when the element was reused."
+        );
+    }
 }
 
 #[cfg(feature = "wasm_test")]

packages/yew/src/dom_bundle/blist.rs

@@ -474,7 +474,7 @@ mod feat_hydration {
                 let (child_node_ref, child) = child.hydrate(root, parent_scope, parent, fragment);
 
                 if index == 0 {
-                    node_ref.reuse(child_node_ref);
+                    node_ref.link(child_node_ref);
                 }
 
                 children.push(child);

packages/yew/src/dom_bundle/utils.rs

@@ -5,7 +5,10 @@ pub(super) fn insert_node(node: &Node, parent: &Element, next_sibling: Option<&N
     match next_sibling {
         Some(next_sibling) => parent
             .insert_before(node, Some(next_sibling))
-            .expect("failed to insert tag before next sibling"),
+            .unwrap_or_else(|err| {
+                gloo::console::error!("failed to insert node", err, parent, next_sibling, node);
+                panic!("failed to insert tag before next sibling")
+            }),
         None => parent.append_child(node).expect("failed to append child"),
     };
 }

packages/yew/src/html/component/lifecycle.rs

@@ -305,7 +305,7 @@ pub(crate) enum UpdateEvent {
     Message,
     /// Wraps properties, node ref, and next sibling for a component
     #[cfg(feature = "csr")]
-    Properties(Rc<dyn Any>, NodeRef, NodeRef),
+    Properties(Rc<dyn Any>, NodeRef),
 }
 
 pub(crate) struct UpdateRunner {
@@ -320,16 +320,13 @@ impl Runnable for UpdateRunner {
                 UpdateEvent::Message => state.inner.flush_messages(),
 
                 #[cfg(feature = "csr")]
-                UpdateEvent::Properties(props, next_node_ref, next_sibling) => {
+                UpdateEvent::Properties(props, next_sibling) => {
                     match state.render_state {
                         #[cfg(feature = "csr")]
                         ComponentRenderState::Render {
-                            ref mut node_ref,
                             next_sibling: ref mut current_next_sibling,
                             ..
                         } => {
-                            // When components are updated, a new node ref could have been passed in
-                            *node_ref = next_node_ref;
                             // When components are updated, their siblings were likely also updated
                             *current_next_sibling = next_sibling;
                             // Only trigger changed if props were changed
@@ -338,12 +335,9 @@ impl Runnable for UpdateRunner {
 
                         #[cfg(feature = "hydration")]
                         ComponentRenderState::Hydration {
-                            ref mut node_ref,
                             next_sibling: ref mut current_next_sibling,
                             ..
                         } => {
-                            // When components are updated, a new node ref could have been passed in
-                            *node_ref = next_node_ref;
                             // When components are updated, their siblings were likely also updated
                             *current_next_sibling = next_sibling;
                             // Only trigger changed if props were changed

packages/yew/src/html/component/scope.rs

@@ -457,16 +457,11 @@ mod feat_csr {
             scheduler::start();
         }
 
-        pub(crate) fn reuse(
+        pub(crate) fn reuse(&self, props: Rc<COMP::Properties>, next_sibling: NodeRef) {
-            &self,
-            props: Rc<COMP::Properties>,
-            node_ref: NodeRef,
-            next_sibling: NodeRef,
-        ) {
             #[cfg(debug_assertions)]
             super::super::log_event(self.id, "reuse");
 
-            self.push_update(UpdateEvent::Properties(props, node_ref, next_sibling));
+            self.push_update(UpdateEvent::Properties(props, next_sibling));
         }
     }
 

packages/yew/src/html/mod.rs

@@ -146,9 +146,9 @@ mod feat_csr {
             }
 
             let mut this = self.0.borrow_mut();
-            let existing = node_ref.0.borrow();
+            let mut existing = node_ref.0.borrow_mut();
-            this.node = existing.node.clone();
+            this.node = existing.node.take();
-            this.link = existing.link.clone();
+            this.link = existing.link.take();
         }
 
         /// Link a downstream `NodeRef`

packages/yew/src/virtual_dom/vcomp.rs

@@ -65,7 +65,7 @@ pub(crate) trait Mountable {
     ) -> Box<dyn Scoped>;
 
     #[cfg(feature = "csr")]
-    fn reuse(self: Box<Self>, node_ref: NodeRef, scope: &dyn Scoped, next_sibling: NodeRef);
+    fn reuse(self: Box<Self>, scope: &dyn Scoped, next_sibling: NodeRef);
 
     #[cfg(feature = "ssr")]
     fn render_to_string<'a>(
@@ -120,9 +120,9 @@ impl<COMP: BaseComponent> Mountable for PropsWrapper<COMP> {
     }
 
     #[cfg(feature = "csr")]
-    fn reuse(self: Box<Self>, node_ref: NodeRef, scope: &dyn Scoped, next_sibling: NodeRef) {
+    fn reuse(self: Box<Self>, scope: &dyn Scoped, next_sibling: NodeRef) {
         let scope: Scope<COMP> = scope.to_any().downcast::<COMP>();
-        scope.reuse(self.props, node_ref, next_sibling);
+        scope.reuse(self.props, next_sibling);
     }
 
     #[cfg(feature = "ssr")]