Skip to content

Commit

Permalink
Fix: Hydratation of empty lists next to components. (#3630)
Browse files Browse the repository at this point in the history
* fix: hydration of empty VLists

an additional (empty) VText was inserted in the second reconciliation
pass, which tried to insert itself in an invalid position. This internal
error was masked by a "fix" of the internal slot of components, which
should have still been trapped to signal that the second fixup render
was not yet run.

* fixup: new lints from clippy
  • Loading branch information
WorldSEnder committed Mar 12, 2024
1 parent 46ebdc8 commit 95c29cc
Show file tree
Hide file tree
Showing 9 changed files with 107 additions and 32 deletions.
87 changes: 84 additions & 3 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion packages/yew/src/callback.rs
Expand Up @@ -25,7 +25,9 @@ macro_rules! generate_callback_impls {
}
}

#[allow(clippy::vtable_address_comparisons)]
// We are okay with comparisons from different compilation units to result in false
// not-equal results. This should only lead in the worst-case to some unneeded re-renders.
#[allow(ambiguous_wide_pointer_comparisons)]
impl<IN, OUT> PartialEq for $callback<IN, OUT> {
fn eq(&self, other: &$callback<IN, OUT>) -> bool {
let ($callback { cb }, $callback { cb: rhs_cb }) = (self, other);
Expand Down
12 changes: 2 additions & 10 deletions packages/yew/src/dom_bundle/blist.rs
Expand Up @@ -11,7 +11,7 @@ use super::{test_log, BNode, BSubtree, DomSlot};
use crate::dom_bundle::{Reconcilable, ReconcileTarget};
use crate::html::AnyScope;
use crate::utils::RcExt;
use crate::virtual_dom::{Key, VList, VNode, VText};
use crate::virtual_dom::{Key, VList, VNode};

/// This struct represents a mounted [VList]
#[derive(Debug)]
Expand Down Expand Up @@ -427,15 +427,7 @@ impl Reconcilable for VList {
// The left items are known since we want to insert them
// (self.children). For the right ones, we will look at the bundle,
// i.e. the current DOM list element that we want to replace with self.
let (key, mut fully_keyed, mut lefts) = self.split_for_blist();

if lefts.is_empty() {
// Without a placeholder the next element becomes first
// and corrupts the order of rendering
// We use empty text element to stake out a place
lefts.push(VText::new("").into());
fully_keyed = false;
}
let (key, fully_keyed, lefts) = self.split_for_blist();

let rights = &mut blist.rev_children;
test_log!("lefts: {:?}", lefts);
Expand Down
9 changes: 4 additions & 5 deletions packages/yew/src/dom_bundle/utils.rs
Expand Up @@ -50,6 +50,10 @@ mod feat_hydration {

#[cfg(feature = "hydration")]
pub(super) use feat_hydration::*;
#[cfg(test)]
// this is needed because clippy doesn't like the import not being used
#[allow(unused_imports)]
pub(super) use tests::*;

#[cfg(test)]
mod tests {
Expand Down Expand Up @@ -87,8 +91,3 @@ mod tests {
(root, scope, parent, sibling)
}
}

#[cfg(test)]
// this is needed because clippy doesn't like the import not being used
#[allow(unused_imports)]
pub(super) use tests::*;
5 changes: 4 additions & 1 deletion packages/yew/src/functional/hooks/use_reducer.rs
Expand Up @@ -130,7 +130,10 @@ where
T: Reducible,
{
fn eq(&self, rhs: &Self) -> bool {
#[allow(clippy::vtable_address_comparisons)]
// We are okay with comparisons from different compilation units to result in false
// not-equal results. This should only lead in the worst-case to some unneeded
// re-renders.
#[allow(ambiguous_wide_pointer_comparisons)]
Rc::ptr_eq(&self.dispatch, &rhs.dispatch)
}
}
Expand Down
4 changes: 3 additions & 1 deletion packages/yew/src/html/component/lifecycle.rs
Expand Up @@ -430,7 +430,9 @@ impl ComponentState {
fields(component.id = self.comp_id)
)]
fn render(&mut self, shared_state: &Shared<Option<ComponentState>>) {
match self.inner.view() {
let view = self.inner.view();
tracing::trace!(?view, "render result");
match view {
Ok(vnode) => self.commit_render(shared_state, vnode),
Err(RenderError::Suspended(susp)) => self.suspend(shared_state, susp),
};
Expand Down
8 changes: 1 addition & 7 deletions packages/yew/src/html/component/scope.rs
Expand Up @@ -642,7 +642,7 @@ mod feat_hydration {
use web_sys::{Element, HtmlScriptElement};

use super::*;
use crate::dom_bundle::{BSubtree, DomSlot, DynamicDomSlot, Fragment};
use crate::dom_bundle::{BSubtree, DynamicDomSlot, Fragment};
use crate::html::component::lifecycle::{ComponentRenderState, CreateRunner, RenderRunner};
use crate::scheduler;
use crate::virtual_dom::Collectable;
Expand Down Expand Up @@ -679,12 +679,6 @@ mod feat_hydration {
let collectable = Collectable::for_component::<COMP>();

let mut fragment = Fragment::collect_between(fragment, &collectable, &parent);
let next_sibling = if let Some(n) = fragment.front() {
Some(n.clone())
} else {
fragment.sibling_at_end().cloned()
};
internal_ref.reassign(DomSlot::create(next_sibling));

let prepared_state = match fragment
.back()
Expand Down
2 changes: 1 addition & 1 deletion packages/yew/src/suspense/suspension.rs
Expand Up @@ -105,7 +105,7 @@ impl Future for Suspension {

let waker = cx.waker().clone();
self.listen(Callback::from(move |_| {
waker.clone().wake();
waker.wake_by_ref();
}));

Poll::Pending
Expand Down
8 changes: 5 additions & 3 deletions packages/yew/src/virtual_dom/listeners.rs
Expand Up @@ -187,9 +187,11 @@ impl PartialEq for Listeners {
lhs.iter()
.zip(rhs.iter())
.all(|(lhs, rhs)| match (lhs, rhs) {
(Some(lhs), Some(rhs)) =>
{
#[allow(clippy::vtable_address_comparisons)]
(Some(lhs), Some(rhs)) => {
// We are okay with comparisons from different compilation units to
// result in false not-equal results. This should only lead in the
// worst-case to some unneeded re-renders.
#[allow(ambiguous_wide_pointer_comparisons)]
Rc::ptr_eq(lhs, rhs)
}
(None, None) => true,
Expand Down

0 comments on commit 95c29cc

Please sign in to comment.