Skip to content

Commit a3a3ffc

Browse files
authored
Fix DomSlot debugging (#3817)
* Fix panic when formatting trapped DomSlot for debugging * add debug print test case
1 parent 550b2cc commit a3a3ffc

File tree

1 file changed

+65
-29
lines changed

1 file changed

+65
-29
lines changed

packages/yew/src/dom_bundle/position.rs

Lines changed: 65 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -30,11 +30,12 @@ pub(crate) struct DynamicDomSlot {
3030
impl std::fmt::Debug for DomSlot {
3131
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
3232
self.with_next_sibling(|n| {
33-
write!(
34-
f,
35-
"DomSlot {{ next_sibling: {:?} }}",
36-
n.map(crate::utils::print_node)
37-
)
33+
let formatted_node = match n {
34+
None => None,
35+
Some(n) if trap_impl::is_trap(n) => Some("<not yet initialized />".to_string()),
36+
Some(n) => Some(crate::utils::print_node(n)),
37+
};
38+
write!(f, "DomSlot {{ next_sibling: {formatted_node:?} }}")
3839
})
3940
}
4041
}
@@ -45,10 +46,37 @@ impl std::fmt::Debug for DynamicDomSlot {
4546
}
4647
}
4748

48-
#[cfg(debug_assertions)]
49-
thread_local! {
50-
// A special marker element that should not be referenced
51-
static TRAP: Node = gloo::utils::document().create_element("div").unwrap().into();
49+
mod trap_impl {
50+
use super::Node;
51+
#[cfg(debug_assertions)]
52+
thread_local! {
53+
// A special marker element that should not be referenced
54+
static TRAP: Node = gloo::utils::document().create_element("div").unwrap().into();
55+
}
56+
/// Get a "trap" node, or None if compiled without debug_assertions
57+
pub fn get_trap_node() -> Option<Node> {
58+
#[cfg(debug_assertions)]
59+
{
60+
TRAP.with(|trap| Some(trap.clone()))
61+
}
62+
#[cfg(not(debug_assertions))]
63+
{
64+
None
65+
}
66+
}
67+
#[inline]
68+
pub fn is_trap(node: &Node) -> bool {
69+
#[cfg(debug_assertions)]
70+
{
71+
TRAP.with(|trap| node == trap)
72+
}
73+
#[cfg(not(debug_assertions))]
74+
{
75+
// When not running with debug_assertions, there is no trap node
76+
let _ = node;
77+
false
78+
}
79+
}
5280
}
5381

5482
impl DomSlot {
@@ -71,41 +99,38 @@ impl DomSlot {
7199
/// A new "placeholder" [DomSlot] that should not be used to insert nodes
72100
#[inline]
73101
pub fn new_debug_trapped() -> Self {
74-
#[cfg(debug_assertions)]
75-
{
76-
Self::at(TRAP.with(|trap| trap.clone()))
77-
}
78-
#[cfg(not(debug_assertions))]
79-
{
80-
Self::at_end()
81-
}
102+
Self::create(trap_impl::get_trap_node())
82103
}
83104

84105
/// Get the [Node] that comes just after the position, or `None` if this denotes the position at
85106
/// the end
86-
fn with_next_sibling<R>(&self, f: impl FnOnce(Option<&Node>) -> R) -> R {
107+
fn with_next_sibling_check_trap<R>(&self, f: impl FnOnce(Option<&Node>) -> R) -> R {
87108
let checkedf = |node: Option<&Node>| {
88-
#[cfg(debug_assertions)]
89-
TRAP.with(|trap| {
90-
assert!(
91-
node != Some(trap),
92-
"Should not use a trapped DomSlot. Please report this as an internal bug in \
93-
yew."
94-
)
95-
});
109+
// MSRV 1.82 could rewrite this with `is_none_or`
110+
let is_trapped = match node {
111+
None => false,
112+
Some(node) => trap_impl::is_trap(node),
113+
};
114+
assert!(
115+
!is_trapped,
116+
"Should not use a trapped DomSlot. Please report this as an internal bug in yew."
117+
);
96118
f(node)
97119
};
120+
self.with_next_sibling(checkedf)
121+
}
98122

123+
fn with_next_sibling<R>(&self, f: impl FnOnce(Option<&Node>) -> R) -> R {
99124
match &self.variant {
100-
DomSlotVariant::Node(ref n) => checkedf(n.as_ref()),
101-
DomSlotVariant::Chained(ref chain) => chain.with_next_sibling(checkedf),
125+
DomSlotVariant::Node(ref n) => f(n.as_ref()),
126+
DomSlotVariant::Chained(ref chain) => chain.with_next_sibling(f),
102127
}
103128
}
104129

105130
/// Insert a [Node] at the position denoted by this slot. `parent` must be the actual parent
106131
/// element of the children that this slot is implicitly a part of.
107132
pub(super) fn insert(&self, parent: &Element, node: &Node) {
108-
self.with_next_sibling(|next_sibling| {
133+
self.with_next_sibling_check_trap(|next_sibling: Option<&Node>| {
109134
parent
110135
.insert_before(node, next_sibling)
111136
.unwrap_or_else(|err| {
@@ -253,4 +278,15 @@ mod layout_tests {
253278
"expected {target:#?} to point to the same position as {replacement:#?}"
254279
);
255280
}
281+
282+
#[test]
283+
fn debug_printing() {
284+
// basic tests that these don't panic. We don't enforce any specific format.
285+
println!("At end: {:?}", DomSlot::at_end());
286+
println!("Trapped: {:?}", DomSlot::new_debug_trapped());
287+
println!(
288+
"At element: {:?}",
289+
DomSlot::at(document().create_element("p").unwrap().into())
290+
);
291+
}
256292
}

0 commit comments

Comments
 (0)