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

Do not detach child elements if parent element is about to be detached #2420

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
c1ed876
Make a use_hook hook with the new Hook trait.
futursolo Jan 12, 2022
863fa92
Merge branch 'master' into fc-hook-v2
futursolo Jan 12, 2022
22d91f6
Implement Lifetime.
futursolo Jan 13, 2022
d940a7d
Rewrites function signature.
futursolo Jan 13, 2022
572becb
Only apply lifetime if there're other lifetimes.
futursolo Jan 16, 2022
7dc8373
Merge branch 'master' into fc-hook-v2
futursolo Jan 22, 2022
7e8bcdd
Cleanup signature rewrite logic.
futursolo Jan 22, 2022
2664141
Rewrite hook body.
futursolo Jan 22, 2022
aac78b1
Port some built-in hooks.
futursolo Jan 22, 2022
1e5c373
Finish porting all built-in hooks.
futursolo Jan 22, 2022
5f086e8
Port tests.
futursolo Jan 22, 2022
a87b2c7
Fix tests.
futursolo Jan 22, 2022
62396f0
Migrate to macro-based hooks.
futursolo Jan 22, 2022
72518e2
Fix HookContext, add tests on non-possible locations.
futursolo Jan 22, 2022
38e06b1
Fix stderr for trybuild.
futursolo Jan 22, 2022
19af0b3
Add 1 more test case.
futursolo Jan 22, 2022
49454cd
Adjust doc location.
futursolo Jan 22, 2022
eb85f47
Pretty print hook signature.
futursolo Jan 22, 2022
4f0260c
Fix Items & std::ops::Fn*.
futursolo Jan 22, 2022
344ae10
Add use_memo.
futursolo Jan 22, 2022
ffca655
Optimise Implementation of hooks.
futursolo Jan 22, 2022
7b8978b
Use Box to capture function value only.
futursolo Jan 22, 2022
15585ff
Detect whether needs boxing.
futursolo Jan 22, 2022
3dbcdac
Merge commit '38021e31fe97202b112c952c6f5fb5b06289fbc5' into fc-hook-v2
futursolo Jan 23, 2022
f07c648
Add args if boxing not needed.
futursolo Jan 23, 2022
7118b19
Enforce hook number.
futursolo Jan 23, 2022
171e085
Deduplicate use_effect.
futursolo Jan 23, 2022
2f3856a
Optimise Implementation.
futursolo Jan 23, 2022
2b1d6f1
Update documentation.
futursolo Jan 23, 2022
ac765cb
Fix website test. Strip BoxedHook implementation from it.
futursolo Jan 23, 2022
9c300ed
Allow doc string.
futursolo Jan 23, 2022
76da3f6
Workaround doc tests.
futursolo Jan 23, 2022
541a945
Optimise codebase & documentation.
futursolo Jan 24, 2022
80dc7e2
Fix website test.
futursolo Jan 24, 2022
347623b
Reduce implementation complexity.
futursolo Jan 25, 2022
e974dff
Destructor is no more.
futursolo Jan 25, 2022
38ef990
Do not detach child element if parent element is about to be detached…
futursolo Jan 25, 2022
fb1d947
Merge branch 'master' into no-child-detaching-if-parent-detached
futursolo Jan 28, 2022
461cf38
Fix tests.
futursolo Jan 29, 2022
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
2 changes: 1 addition & 1 deletion packages/yew/src/app_handle.rs
Expand Up @@ -35,7 +35,7 @@ where

/// Schedule the app for destruction
pub fn destroy(mut self) {
self.scope.destroy()
self.scope.destroy(false)
}
}

Expand Down
3 changes: 2 additions & 1 deletion packages/yew/src/html/component/lifecycle.rs
Expand Up @@ -179,6 +179,7 @@ impl<COMP: BaseComponent> Runnable for UpdateRunner<COMP> {

pub(crate) struct DestroyRunner<COMP: BaseComponent> {
pub(crate) state: Shared<Option<ComponentState<COMP>>>,
pub(crate) parent_to_detach: bool,
}

impl<COMP: BaseComponent> Runnable for DestroyRunner<COMP> {
Expand All @@ -190,7 +191,7 @@ impl<COMP: BaseComponent> Runnable for DestroyRunner<COMP> {
state.component.destroy(&state.context);

if let Some(ref m) = state.parent {
state.root_node.detach(m);
state.root_node.detach(m, self.parent_to_detach);
state.node_ref.set(None);
}
}
Expand Down
6 changes: 4 additions & 2 deletions packages/yew/src/html/component/scope.rs
Expand Up @@ -115,7 +115,7 @@ impl AnyScope {
pub(crate) trait Scoped {
fn to_any(&self) -> AnyScope;
fn root_vnode(&self) -> Option<Ref<'_, VNode>>;
fn destroy(&mut self);
fn destroy(&mut self, parent_to_detach: bool);
fn shift_node(&self, parent: Element, next_sibling: NodeRef);
}

Expand All @@ -136,9 +136,10 @@ impl<COMP: BaseComponent> Scoped for Scope<COMP> {
}

/// Process an event to destroy a component
fn destroy(&mut self) {
fn destroy(&mut self, parent_to_detach: bool) {
scheduler::push_component_destroy(DestroyRunner {
state: self.state.clone(),
parent_to_detach,
});
// Not guaranteed to already have the scheduler started
scheduler::start();
Expand Down Expand Up @@ -387,6 +388,7 @@ mod feat_ssr {

scheduler::push_component_destroy(DestroyRunner {
state: self.state.clone(),
parent_to_detach: false,
});
scheduler::start();
}
Expand Down
4 changes: 3 additions & 1 deletion packages/yew/src/virtual_dom/mod.rs
Expand Up @@ -497,7 +497,9 @@ impl Default for Attributes {
/// This trait provides features to update a tree by calculating a difference against another tree.
pub(crate) trait VDiff {
/// Remove self from parent.
fn detach(&mut self, parent: &Element);
///
/// Parent to detach is `true` if the parent element will also be detached.
fn detach(&mut self, parent: &Element, parent_to_detach: bool);

/// Move elements from one parent to another parent.
/// This is currently only used by `VSuspense` to preserve component state without detaching
Expand Down
8 changes: 4 additions & 4 deletions packages/yew/src/virtual_dom/vcomp.rs
Expand Up @@ -243,8 +243,8 @@ impl<COMP: BaseComponent> Mountable for PropsWrapper<COMP> {
}

impl VDiff for VComp {
fn detach(&mut self, _parent: &Element) {
self.take_scope().destroy();
fn detach(&mut self, _parent: &Element, parent_to_detach: bool) {
self.take_scope().destroy(parent_to_detach);
}

fn shift(&self, _previous_parent: &Element, next_parent: &Element, next_sibling: NodeRef) {
Expand Down Expand Up @@ -276,7 +276,7 @@ impl VDiff for VComp {
}
}

ancestor.detach(parent);
ancestor.detach(parent, false);
}

self.scope = Some(mountable.mount(
Expand Down Expand Up @@ -595,7 +595,7 @@ mod tests {
elem.apply(&scope, &parent, NodeRef::default(), None);
let parent_node = parent.deref();
assert_eq!(node_ref.get(), parent_node.first_child());
elem.detach(&parent);
elem.detach(&parent, false);
assert!(node_ref.get().is_none());
}
}
Expand Down
8 changes: 4 additions & 4 deletions packages/yew/src/virtual_dom/vlist.rs
Expand Up @@ -154,7 +154,7 @@ impl VList {
while diff < 0 {
let mut r = rights_it.next().unwrap();
test_log!("removing: {:?}", r);
r.detach(parent);
r.detach(parent, false);
diff += 1;
}

Expand Down Expand Up @@ -268,7 +268,7 @@ impl VList {
// Remove any extra rights
for (_, (mut r, _)) in rights_diff.drain() {
test_log!("removing: {:?}", r);
r.detach(parent);
r.detach(parent, false);
}

// Diff matching children at the start
Expand Down Expand Up @@ -307,9 +307,9 @@ mod feat_ssr {
}

impl VDiff for VList {
fn detach(&mut self, parent: &Element) {
fn detach(&mut self, parent: &Element, parent_to_detach: bool) {
for mut child in self.children.drain(..) {
child.detach(parent);
child.detach(parent, parent_to_detach);
}
}

Expand Down
17 changes: 9 additions & 8 deletions packages/yew/src/virtual_dom/vnode.rs
Expand Up @@ -126,19 +126,19 @@ impl VNode {

impl VDiff for VNode {
/// Remove VNode from parent.
fn detach(&mut self, parent: &Element) {
fn detach(&mut self, parent: &Element, parent_to_detach: bool) {
match *self {
VNode::VTag(ref mut vtag) => vtag.detach(parent),
VNode::VText(ref mut vtext) => vtext.detach(parent),
VNode::VComp(ref mut vcomp) => vcomp.detach(parent),
VNode::VList(ref mut vlist) => vlist.detach(parent),
VNode::VTag(ref mut vtag) => vtag.detach(parent, parent_to_detach),
VNode::VText(ref mut vtext) => vtext.detach(parent, parent_to_detach),
VNode::VComp(ref mut vcomp) => vcomp.detach(parent, parent_to_detach),
VNode::VList(ref mut vlist) => vlist.detach(parent, parent_to_detach),
VNode::VRef(ref node) => {
if parent.remove_child(node).is_err() {
console::warn!("Node not found to remove VRef");
}
}
VNode::VPortal(ref mut vportal) => vportal.detach(parent),
VNode::VSuspense(ref mut vsuspense) => vsuspense.detach(parent),
VNode::VPortal(ref mut vportal) => vportal.detach(parent, parent_to_detach),
VNode::VSuspense(ref mut vsuspense) => vsuspense.detach(parent, parent_to_detach),
}
}

Expand Down Expand Up @@ -183,12 +183,13 @@ impl VDiff for VNode {
}
VNode::VRef(ref mut node) => {
if let Some(mut ancestor) = ancestor {
// We always remove VRef in case it's meant to be used somewhere else.
if let VNode::VRef(n) = &ancestor {
if node == n {
return NodeRef::new(node.clone());
}
}
ancestor.detach(parent);
ancestor.detach(parent, false);
}
super::insert_node(node, parent, next_sibling.get().as_ref());
NodeRef::new(node.clone())
Expand Down
8 changes: 4 additions & 4 deletions packages/yew/src/virtual_dom/vportal.rs
Expand Up @@ -17,8 +17,8 @@ pub struct VPortal {
}

impl VDiff for VPortal {
fn detach(&mut self, _: &Element) {
self.node.detach(&self.host);
fn detach(&mut self, _: &Element, _parent_to_detach: bool) {
self.node.detach(&self.host, false);
self.sibling_ref.set(None);
}

Expand All @@ -43,7 +43,7 @@ impl VDiff for VPortal {
} = old_portal;
if old_host != self.host {
// Remount the inner node somewhere else instead of diffing
node.detach(&old_host);
node.detach(&old_host, false);
None
} else if old_sibling != self.next_sibling {
// Move the node, but keep the state
Expand All @@ -54,7 +54,7 @@ impl VDiff for VPortal {
}
}
Some(mut node) => {
node.detach(parent);
node.detach(parent, false);
None
}
None => None,
Expand Down
14 changes: 7 additions & 7 deletions packages/yew/src/virtual_dom/vsuspense.rs
Expand Up @@ -48,14 +48,14 @@ impl VSuspense {
}

impl VDiff for VSuspense {
fn detach(&mut self, parent: &Element) {
fn detach(&mut self, parent: &Element, parent_to_detach: bool) {
if self.suspended {
self.fallback.detach(parent);
self.fallback.detach(parent, parent_to_detach);
if let Some(ref m) = self.detached_parent {
self.children.detach(m);
self.children.detach(m, false);
}
} else {
self.children.detach(parent);
self.children.detach(parent, parent_to_detach);
}
}

Expand All @@ -82,15 +82,15 @@ impl VDiff for VSuspense {
Some(VNode::VSuspense(mut m)) => {
// We only preserve the child state if they are the same suspense.
if m.key != self.key || self.detached_parent != m.detached_parent {
m.detach(parent);
m.detach(parent, false);

(false, None, None)
} else {
(m.suspended, Some(*m.children), Some(*m.fallback))
}
}
Some(mut m) => {
m.detach(parent);
m.detach(parent, false);
(false, None, None)
}
None => (false, None, None),
Expand Down Expand Up @@ -136,7 +136,7 @@ impl VDiff for VSuspense {
}

(false, true) => {
fallback_ancestor.unwrap().detach(parent);
fallback_ancestor.unwrap().detach(parent, false);

children_ancestor.as_ref().unwrap().shift(
detached_parent,
Expand Down
19 changes: 12 additions & 7 deletions packages/yew/src/virtual_dom/vtag.rs
Expand Up @@ -476,7 +476,7 @@ impl VTag {

impl VDiff for VTag {
/// Remove VTag from parent.
fn detach(&mut self, parent: &Element) {
fn detach(&mut self, parent: &Element, parent_to_detach: bool) {
let node = self
.reference
.take()
Expand All @@ -486,10 +486,15 @@ impl VDiff for VTag {

// recursively remove its children
if let VTagInner::Other { children, .. } = &mut self.inner {
children.detach(&node);
// This tag will be removed, so there's no point to remove any child.
children.detach(&node, true);
}
if parent.remove_child(&node).is_err() {
console::warn!("Node not found to remove VTag");
if !parent_to_detach {
let result = parent.remove_child(&node);

if result.is_err() {
console::warn!("Node not found to remove VTag");
}
}
// It could be that the ref was already reused when rendering another element.
// Only unset the ref it still belongs to our node
Expand Down Expand Up @@ -555,7 +560,7 @@ impl VDiff for VTag {
} else {
let el = self.create_element(parent);
super::insert_node(&el, parent, ancestor.first_node().as_ref());
ancestor.detach(parent);
ancestor.detach(parent, false);
(None, el)
}
}
Expand Down Expand Up @@ -605,7 +610,7 @@ impl VDiff for VTag {
if !new.is_empty() {
new.apply(parent_scope, &el, NodeRef::default(), Some(old.into()));
} else if !old.is_empty() {
old.detach(&el);
old.detach(&el, false);
}
}
// Can not happen, because we checked for tag equability above
Expand Down Expand Up @@ -1191,7 +1196,7 @@ mod tests {
elem.apply(&scope, &parent, NodeRef::default(), None);
let parent_node = parent.deref();
assert_eq!(node_ref.get(), parent_node.first_child());
elem.detach(&parent);
elem.detach(&parent, false);
assert!(node_ref.get().is_none());
}

Expand Down
12 changes: 8 additions & 4 deletions packages/yew/src/virtual_dom/vtext.rs
Expand Up @@ -55,13 +55,17 @@ impl std::fmt::Debug for VText {

impl VDiff for VText {
/// Remove VText from parent.
fn detach(&mut self, parent: &Element) {
fn detach(&mut self, parent: &Element, parent_to_detach: bool) {
let node = self
.reference
.take()
.expect("tried to remove not rendered VText from DOM");
if parent.remove_child(&node).is_err() {
console::warn!("Node not found to remove VText");
if !parent_to_detach {
let result = parent.remove_child(&node);

if result.is_err() {
console::warn!("Node not found to remove VText");
}
}
}

Expand Down Expand Up @@ -99,7 +103,7 @@ impl VDiff for VText {
return NodeRef::new(text_node.into());
}

ancestor.detach(parent);
ancestor.detach(parent, false);
}

let text_node = document().create_text_node(&self.text);
Expand Down