Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue with node refs and hydration #2597

Merged
merged 2 commits into from Apr 12, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
50 changes: 42 additions & 8 deletions packages/yew/src/dom_bundle/bcomp.rs
Expand Up @@ -13,6 +13,7 @@ pub(super) struct BComp {
type_id: TypeId,
scope: Box<dyn Scoped>,
node_ref: NodeRef,
internal_ref: NodeRef,
key: Option<Key>,
}

Expand Down Expand Up @@ -57,20 +58,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,
},
Expand Down Expand Up @@ -112,10 +116,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);
node_ref
mountable.reuse(bcomp.scope.borrow(), next_sibling);
bcomp.internal_ref.clone()
}
}

Expand All @@ -139,21 +143,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,
},
)
Expand All @@ -165,7 +172,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,
Expand Down Expand Up @@ -442,6 +449,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")]
Expand Down
2 changes: 1 addition & 1 deletion packages/yew/src/dom_bundle/blist.rs
Expand Up @@ -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);
Expand Down
5 changes: 4 additions & 1 deletion packages/yew/src/dom_bundle/utils.rs
Expand Up @@ -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"),
};
}
Expand Down
10 changes: 2 additions & 8 deletions packages/yew/src/html/component/lifecycle.rs
Expand Up @@ -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 {
Expand All @@ -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
Expand All @@ -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
Expand Down
9 changes: 2 additions & 7 deletions packages/yew/src/html/component/scope.rs
Expand Up @@ -457,16 +457,11 @@ mod feat_csr {
scheduler::start();
}

pub(crate) fn reuse(
&self,
props: Rc<COMP::Properties>,
node_ref: NodeRef,
next_sibling: NodeRef,
) {
pub(crate) fn reuse(&self, props: Rc<COMP::Properties>, 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));
}
}

Expand Down
6 changes: 3 additions & 3 deletions packages/yew/src/html/mod.rs
Expand Up @@ -146,9 +146,9 @@ mod feat_csr {
}

let mut this = self.0.borrow_mut();
let existing = node_ref.0.borrow();
this.node = existing.node.clone();
this.link = existing.link.clone();
let mut existing = node_ref.0.borrow_mut();
this.node = existing.node.take();
this.link = existing.link.take();
}

/// Link a downstream `NodeRef`
Expand Down
6 changes: 3 additions & 3 deletions packages/yew/src/virtual_dom/vcomp.rs
Expand Up @@ -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>(
Expand Down Expand Up @@ -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")]
Expand Down