Skip to content

Commit 4021951

Browse files
fix inconsistent effects for suspended nodes (#3991)
The previous approach routed a resuming child's rendered through the scheduler's `rendered` queue, which drains only after the update+render queues. That worked for a single suspender but not for siblings: if A resumed while B was still pending, A's rendered still fired before the boundary un-suspended, because Suspense's render with suspended=true still doesn't shift children into the live tree. Hand the pending rendered directly to the ancestor BaseSuspense as a PendingRendered, keyed by comp_id in a Vec. BaseSuspense drains this list in its own rendered lifecycle, gated on self.suspensions.is_empty() - i.e. the commit on which reconcile has already shifted children from the detached parent into the live tree. Re-committed children absorb into the existing entry so first_render=true survives a suspend/resume/suspend/resume cycle.
1 parent 28219ae commit 4021951

4 files changed

Lines changed: 294 additions & 10 deletions

File tree

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

Lines changed: 65 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -487,6 +487,8 @@ impl ComponentState {
487487
fn commit_render(&mut self, shared_state: &Shared<Option<ComponentState>>, new_vdom: Html) {
488488
// Currently not suspended, we remove any previous suspension and update
489489
// normally.
490+
#[cfg(feature = "csr")]
491+
let resuming_from_suspension = self.suspension.is_some();
490492
self.resume_existing_suspension();
491493

492494
match self.render_state {
@@ -508,14 +510,28 @@ impl ComponentState {
508510
let first_render = !self.has_rendered;
509511
self.has_rendered = true;
510512

511-
scheduler::push_component_rendered(
512-
self.comp_id,
513-
Box::new(RenderedRunner {
514-
state: shared_state.clone(),
513+
if resuming_from_suspension {
514+
// The DOM we just reconciled still lives in the ancestor
515+
// Suspense's detached parent. The Suspense must process
516+
// the Resume message and re-render to shift children into
517+
// the live tree before our effects observe the DOM.
518+
// Hand the pending `rendered` to the Suspense; it will
519+
// re-schedule it once it fully un-suspends.
520+
let pending = PendingRendered::new(shared_state.clone(), first_render);
521+
let suspense_scope = scope
522+
.find_parent_scope::<BaseSuspense>()
523+
.expect("a resuming component must have a Suspense ancestor");
524+
BaseSuspense::defer_rendered(&suspense_scope, self.comp_id, pending);
525+
} else {
526+
scheduler::push_component_rendered(
527+
self.comp_id,
528+
Box::new(RenderedRunner {
529+
state: shared_state.clone(),
530+
first_render,
531+
}),
515532
first_render,
516-
}),
517-
first_render,
518-
);
533+
);
534+
}
519535
}
520536

521537
#[cfg(feature = "hydration")]
@@ -701,6 +717,47 @@ mod feat_csr {
701717
pub first_render: bool,
702718
}
703719

720+
/// A `rendered` lifecycle deferred by the ancestor `<Suspense>` so it fires
721+
/// only after Suspense un-suspends and children's DOM has been shifted into
722+
/// the live tree. See `BaseSuspense::defer_rendered`.
723+
pub(crate) struct PendingRendered {
724+
pub state: Shared<Option<ComponentState>>,
725+
pub first_render: bool,
726+
}
727+
728+
impl PendingRendered {
729+
pub(crate) fn new(state: Shared<Option<ComponentState>>, first_render: bool) -> Self {
730+
Self {
731+
state,
732+
first_render,
733+
}
734+
}
735+
736+
/// Absorb a later-committed pending rendered for the same component:
737+
/// keep the latest state but preserve `first_render=true` if either
738+
/// side carried it.
739+
pub(crate) fn absorb(&mut self, later: Self) {
740+
self.state = later.state;
741+
self.first_render |= later.first_render;
742+
}
743+
744+
/// Push this onto the scheduler's `rendered` queue.
745+
pub(crate) fn schedule(self, comp_id: usize) {
746+
let PendingRendered {
747+
state,
748+
first_render,
749+
} = self;
750+
scheduler::push_component_rendered(
751+
comp_id,
752+
Box::new(RenderedRunner {
753+
state,
754+
first_render,
755+
}),
756+
false,
757+
);
758+
}
759+
}
760+
704761
impl ComponentState {
705762
#[tracing::instrument(
706763
level = tracing::Level::DEBUG,
@@ -741,7 +798,7 @@ mod feat_csr {
741798
}
742799

743800
#[cfg(feature = "csr")]
744-
pub(super) use feat_csr::*;
801+
pub(crate) use feat_csr::*;
745802

746803
#[cfg(all(target_arch = "wasm32", not(target_os = "wasi")))]
747804
#[cfg(test)]

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@ mod scope;
1010
use std::rc::Rc;
1111

1212
pub use children::*;
13+
#[cfg(feature = "csr")]
14+
pub(crate) use lifecycle::PendingRendered;
1315
pub use marker::*;
1416
pub use properties::*;
1517
#[cfg(feature = "csr")]

packages/yew/src/suspense/component.rs

Lines changed: 61 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,12 @@ pub struct SuspenseProps {
1414

1515
#[cfg(any(feature = "csr", feature = "ssr"))]
1616
mod feat_csr_ssr {
17+
#[cfg(feature = "csr")]
18+
use std::cell::RefCell;
19+
1720
use super::*;
21+
#[cfg(feature = "csr")]
22+
use crate::html::PendingRendered;
1823
use crate::html::{Component, Context, Html, Scope};
1924
use crate::suspense::Suspension;
2025
#[cfg(feature = "hydration")]
@@ -35,11 +40,29 @@ mod feat_csr_ssr {
3540
Resume(Suspension),
3641
}
3742

38-
#[derive(Debug)]
3943
pub(crate) struct BaseSuspense {
4044
suspensions: Vec<Suspension>,
4145
#[cfg(feature = "hydration")]
4246
hydration_handle: Option<SuspensionHandle>,
47+
/// Rendered runners for child components that resumed while this
48+
/// Suspense was still suspended (because of other pending siblings).
49+
/// Drained in `rendered` once the Suspense fully un-suspends, so
50+
/// effects fire only after children's DOM has been shifted into the
51+
/// live tree.
52+
///
53+
/// A small `Vec` is used over a map; the expected population is the
54+
/// number of suspending direct descendants resumed in one boundary
55+
/// transition, typically just a handful.
56+
#[cfg(feature = "csr")]
57+
pending_rendered: RefCell<Vec<(usize, PendingRendered)>>,
58+
}
59+
60+
impl std::fmt::Debug for BaseSuspense {
61+
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
62+
f.debug_struct("BaseSuspense")
63+
.field("suspensions", &self.suspensions)
64+
.finish()
65+
}
4366
}
4467

4568
impl Component for BaseSuspense {
@@ -73,6 +96,8 @@ mod feat_csr_ssr {
7396
suspensions,
7497
#[cfg(feature = "hydration")]
7598
hydration_handle,
99+
#[cfg(feature = "csr")]
100+
pending_rendered: RefCell::new(Vec::new()),
76101
}
77102
}
78103

@@ -128,13 +153,25 @@ mod feat_csr_ssr {
128153
}
129154
}
130155

131-
#[cfg(feature = "hydration")]
132156
fn rendered(&mut self, _ctx: &Context<Self>, first_render: bool) {
157+
#[cfg(not(feature = "hydration"))]
158+
let _ = first_render;
159+
#[cfg(feature = "hydration")]
133160
if first_render {
134161
if let Some(m) = self.hydration_handle.take() {
135162
m.resume();
136163
}
137164
}
165+
// Fire deferred rendered callbacks for children that resumed while
166+
// we were still suspended. Only safe now that we're un-suspended:
167+
// the last reconcile shifted their DOM into the live tree.
168+
#[cfg(feature = "csr")]
169+
if self.suspensions.is_empty() {
170+
let pending = std::mem::take(&mut *self.pending_rendered.borrow_mut());
171+
for (comp_id, p) in pending {
172+
p.schedule(comp_id);
173+
}
174+
}
138175
}
139176
}
140177

@@ -146,6 +183,28 @@ mod feat_csr_ssr {
146183
pub(crate) fn resume(scope: &Scope<Self>, s: Suspension) {
147184
scope.send_message(BaseSuspenseMsg::Resume(s));
148185
}
186+
187+
/// Queue a child component's `rendered` lifecycle to be scheduled once
188+
/// this Suspense fully un-suspends and its reconcile has shifted the
189+
/// child's DOM into the live tree. If the child already has a pending
190+
/// entry (e.g. it re-committed between suspensions), the two are
191+
/// merged so `first_render=true` is not lost.
192+
#[cfg(feature = "csr")]
193+
pub(crate) fn defer_rendered(
194+
scope: &Scope<Self>,
195+
comp_id: usize,
196+
pending: PendingRendered,
197+
) {
198+
let Some(comp) = scope.get_component() else {
199+
return;
200+
};
201+
let mut q = comp.pending_rendered.borrow_mut();
202+
if let Some(slot) = q.iter_mut().find(|(id, _)| *id == comp_id) {
203+
slot.1.absorb(pending);
204+
} else {
205+
q.push((comp_id, pending));
206+
}
207+
}
149208
}
150209

151210
/// Suspend rendering and show a fallback UI until the underlying task completes.

packages/yew/tests/suspense.rs

Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -816,3 +816,169 @@ async fn test_duplicate_suspension() {
816816
let result = obtain_result();
817817
assert_eq!(result.as_str(), "hello!");
818818
}
819+
820+
// Regression test for https://github.com/yewstack/yew/issues/3780
821+
// use_future causes use_effect to fire before DOM is updated, so
822+
// document.get_element_by_id returns None for elements the component renders.
823+
#[wasm_bindgen_test]
824+
async fn use_effect_can_access_dom_after_use_future_resolves() {
825+
#[derive(Properties, Clone)]
826+
struct ContentProps {
827+
dom_observed: Rc<RefCell<Option<bool>>>,
828+
}
829+
830+
impl PartialEq for ContentProps {
831+
fn eq(&self, _other: &Self) -> bool {
832+
true
833+
}
834+
}
835+
836+
#[component(Content)]
837+
fn content(props: &ContentProps) -> HtmlResult {
838+
use_future(|| async {
839+
sleep(Duration::ZERO).await;
840+
})?;
841+
842+
{
843+
let dom_observed = props.dom_observed.clone();
844+
use_effect_with((), move |_| {
845+
let element = gloo::utils::document().get_element_by_id("foo");
846+
*dom_observed.borrow_mut() = Some(element.is_some());
847+
|| {}
848+
});
849+
}
850+
851+
Ok(html! {
852+
<div id="result">
853+
<div id="foo"></div>
854+
</div>
855+
})
856+
}
857+
858+
#[derive(Properties, Clone)]
859+
struct AppProps {
860+
dom_observed: Rc<RefCell<Option<bool>>>,
861+
}
862+
863+
impl PartialEq for AppProps {
864+
fn eq(&self, _other: &Self) -> bool {
865+
true
866+
}
867+
}
868+
869+
#[component(App)]
870+
fn app(props: &AppProps) -> Html {
871+
html! {
872+
<Suspense fallback={html! {<div>{"loading"}</div>}}>
873+
<Content dom_observed={props.dom_observed.clone()} />
874+
</Suspense>
875+
}
876+
}
877+
878+
let dom_observed: Rc<RefCell<Option<bool>>> = Rc::new(RefCell::new(None));
879+
880+
yew::Renderer::<App>::with_root_and_props(
881+
gloo::utils::document().get_element_by_id("output").unwrap(),
882+
AppProps {
883+
dom_observed: dom_observed.clone(),
884+
},
885+
)
886+
.render();
887+
888+
// After everything settles (suspension resolves, component renders, effects run),
889+
// use_effect should have found the #foo element in the DOM.
890+
//
891+
// Bug (issue #3780): use_effect fires before the DOM is updated when use_future
892+
// is involved, so get_element_by_id("foo") returns None and dom_observed is Some(false).
893+
// Expected: use_effect fires after the DOM is committed, so dom_observed should be
894+
// Some(true).
895+
sleep(Duration::from_millis(50)).await;
896+
assert_eq!(
897+
*dom_observed.borrow(),
898+
Some(true),
899+
"use_effect should see the rendered DOM element after use_future resolves"
900+
);
901+
}
902+
903+
// Companion to #3780: when multiple siblings under the same <Suspense> each
904+
// suspend independently and resume at different times, every child's effect
905+
// must see the DOM in the live tree, not the detached parent where Suspense
906+
// parks children while at least one of them is still pending.
907+
#[wasm_bindgen_test]
908+
async fn sibling_suspensions_effects_see_dom() {
909+
#[derive(Properties, Clone)]
910+
struct ChildProps {
911+
id: &'static str,
912+
observed: Rc<RefCell<Vec<(&'static str, bool)>>>,
913+
}
914+
915+
impl PartialEq for ChildProps {
916+
fn eq(&self, other: &Self) -> bool {
917+
self.id == other.id
918+
}
919+
}
920+
921+
#[component(Child)]
922+
fn child(props: &ChildProps) -> HtmlResult {
923+
use_future(|| async {
924+
sleep(Duration::ZERO).await;
925+
})?;
926+
927+
let id = props.id;
928+
let observed = props.observed.clone();
929+
use_effect_with((), move |_| {
930+
let found = gloo::utils::document().get_element_by_id(id).is_some();
931+
observed.borrow_mut().push((id, found));
932+
|| {}
933+
});
934+
935+
Ok(html! { <div id={id}></div> })
936+
}
937+
938+
#[derive(Properties, Clone)]
939+
struct AppProps {
940+
observed: Rc<RefCell<Vec<(&'static str, bool)>>>,
941+
}
942+
943+
impl PartialEq for AppProps {
944+
fn eq(&self, _other: &Self) -> bool {
945+
true
946+
}
947+
}
948+
949+
#[component(App)]
950+
fn app(props: &AppProps) -> Html {
951+
html! {
952+
<Suspense fallback={html! { <div>{"loading"}</div> }}>
953+
<Child id="sib-a" observed={props.observed.clone()} />
954+
<Child id="sib-b" observed={props.observed.clone()} />
955+
</Suspense>
956+
}
957+
}
958+
959+
let observed: Rc<RefCell<Vec<(&'static str, bool)>>> = Rc::new(RefCell::new(Vec::new()));
960+
yew::Renderer::<App>::with_root_and_props(
961+
gloo::utils::document().get_element_by_id("output").unwrap(),
962+
AppProps {
963+
observed: observed.clone(),
964+
},
965+
)
966+
.render();
967+
968+
sleep(Duration::from_millis(100)).await;
969+
970+
let observed = observed.borrow();
971+
assert_eq!(
972+
observed.len(),
973+
2,
974+
"both sibling effects should fire exactly once: {:?}",
975+
*observed
976+
);
977+
for (id, found) in observed.iter() {
978+
assert!(
979+
*found,
980+
"effect for {} did not see DOM in live tree: {:?}",
981+
id, *observed
982+
);
983+
}
984+
}

0 commit comments

Comments
 (0)