From 4d6fb74e873d704c19bebdf8e9fa9b103f1e4508 Mon Sep 17 00:00:00 2001 From: Evan Almloff Date: Thu, 15 Aug 2024 00:18:37 +0200 Subject: [PATCH] Fix hot reloading component properties out of order (#2842) * Fix hot reloading component properties out of order * ignore signal write warnings in hot reloading * Fix hot reloading components with only some literal properties * Add a test for hot reloading component properties --- Cargo.lock | 1 + packages/core/src/hotreload_utils.rs | 23 ++++++++++++++++++----- packages/hot-reload/Cargo.toml | 1 + packages/hot-reload/src/client.rs | 7 ++++++- packages/rsx/src/hot_reload/diff.rs | 15 ++++++++------- packages/rsx/tests/hotreload_pattern.rs | 16 ++++++++++++++-- 6 files changed, 48 insertions(+), 15 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e76b042c7..67d0f6100 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2715,6 +2715,7 @@ dependencies = [ "tokio-stream", "tokio-tungstenite 0.23.1", "tracing", + "warnings", ] [[package]] diff --git a/packages/core/src/hotreload_utils.rs b/packages/core/src/hotreload_utils.rs index 7d8380ffd..643b224f5 100644 --- a/packages/core/src/hotreload_utils.rs +++ b/packages/core/src/hotreload_utils.rs @@ -170,21 +170,34 @@ impl DynamicLiteralPool { *(Box::new(t) as Box).downcast::().unwrap() } let grab_float = || { - self.get_component_property(id, hot_reload, HotReloadLiteral::as_float).expect("Expected a float component property. This is probably caused by a bug in dioxus hot reloading. Please report this issue.") + self.get_component_property(id, hot_reload, HotReloadLiteral::as_float).unwrap_or_else(|| { + tracing::error!("Expected a float component property, because the type was {}. The CLI gave the hot reloading engine a type of {:?}. This is probably caused by a bug in dioxus hot reloading. Please report this issue.", std::any::type_name::(), hot_reload.component_values.get(id)); + Default::default() + + }) }; let grab_int = || { - self.get_component_property(id, hot_reload, HotReloadLiteral::as_int).expect("Expected an int component property. This is probably caused by a bug in dioxus hot reloading. Please report this issue.") + self.get_component_property(id, hot_reload, HotReloadLiteral::as_int).unwrap_or_else(|| { + tracing::error!("Expected a integer component property, because the type was {}. The CLI gave the hot reloading engine a type of {:?}. This is probably caused by a bug in dioxus hot reloading. Please report this issue.", std::any::type_name::(), hot_reload.component_values.get(id)); + Default::default() + }) }; let grab_bool = || { - self.get_component_property(id, hot_reload, HotReloadLiteral::as_bool).expect("Expected a bool component property. This is probably caused by a bug in dioxus hot reloading. Please report this issue.") + self.get_component_property(id, hot_reload, HotReloadLiteral::as_bool).unwrap_or_else(|| { + tracing::error!("Expected a bool component property, because the type was {}. The CLI gave the hot reloading engine a type of {:?}. This is probably caused by a bug in dioxus hot reloading. Please report this issue.", std::any::type_name::(), hot_reload.component_values.get(id)); + Default::default() + }) }; let grab_fmted = || { - self.get_component_property(id, hot_reload, |fmted| HotReloadLiteral::as_fmted(fmted).map(|segments| self.render_formatted(segments))).expect("Expected a string component property. This is probably caused by a bug in dioxus hot reloading. Please report this issue.") + self.get_component_property(id, hot_reload, |fmted| HotReloadLiteral::as_fmted(fmted).map(|segments| self.render_formatted(segments))).unwrap_or_else(|| { + tracing::error!("Expected a string component property, because the type was {}. The CLI gave the hot reloading engine a type of {:?}. This is probably caused by a bug in dioxus hot reloading. Please report this issue.", std::any::type_name::(), hot_reload.component_values.get(id)); + Default::default() + }) }; match TypeId::of::() { // Any string types that accept a literal _ if TypeId::of::() == TypeId::of::() => assert_type(grab_fmted()), - _ if TypeId::of::<&'static str>() == TypeId::of::() => { + _ if TypeId::of::<&str>() == TypeId::of::() => { assert_type(Box::leak(grab_fmted().into_boxed_str()) as &'static str) } // Any integer types that accept a literal diff --git a/packages/hot-reload/Cargo.toml b/packages/hot-reload/Cargo.toml index e8887d371..22c562158 100644 --- a/packages/hot-reload/Cargo.toml +++ b/packages/hot-reload/Cargo.toml @@ -29,6 +29,7 @@ tokio-stream = { version = "0.1.12", features = ["sync"], optional = true } futures-util = { workspace = true, features = ["async-await-macro"], optional = true } tokio = { workspace = true, features = ["sync", "rt-multi-thread"], optional = true } tracing = { workspace = true } +warnings.workspace = true # use rustls on android [target.'cfg(target_os = "android")'.dependencies] diff --git a/packages/hot-reload/src/client.rs b/packages/hot-reload/src/client.rs index 1b0804d86..ca825e517 100644 --- a/packages/hot-reload/src/client.rs +++ b/packages/hot-reload/src/client.rs @@ -1,6 +1,7 @@ use crate::HotReloadMsg; use dioxus_core::{ScopeId, VirtualDom}; use dioxus_signals::Writable; +use warnings::Warning; /// Applies template and literal changes to the VirtualDom /// @@ -13,7 +14,11 @@ pub fn apply_changes(dom: &mut VirtualDom, msg: &HotReloadMsg) { let id = &template.location; let value = template.template.clone(); if let Some(mut signal) = ctx.get_signal_with_key(id) { - signal.set(Some(value)); + dioxus_signals::warnings::signal_read_and_write_in_reactive_scope::allow(|| { + dioxus_signals::warnings::signal_write_in_component_body::allow(|| { + signal.set(Some(value)); + }); + }); } } }); diff --git a/packages/rsx/src/hot_reload/diff.rs b/packages/rsx/src/hot_reload/diff.rs index a6af8e603..628bcc817 100644 --- a/packages/rsx/src/hot_reload/diff.rs +++ b/packages/rsx/src/hot_reload/diff.rs @@ -411,13 +411,14 @@ impl HotReloadResult { } let mut new_fields = new_component.fields.clone(); - new_fields.sort_by(|a, b| a.name.to_string().cmp(&b.name.to_string())); - let mut old_fields = old_component.fields.clone(); - old_fields.sort_by(|a, b| a.name.to_string().cmp(&b.name.to_string())); + new_fields.sort_by_key(|attribute| attribute.name.to_string()); + let mut old_fields = old_component.fields.iter().enumerate().collect::>(); + old_fields.sort_by_key(|(_, attribute)| attribute.name.to_string()); - let mut literal_component_properties = Vec::new(); + // The literal component properties for the component in same the order as the original component property literals + let mut literal_component_properties = vec![None; old_fields.len()]; - for (new_field, old_field) in new_fields.iter().zip(old_fields.iter()) { + for (new_field, (index, old_field)) in new_fields.iter().zip(old_fields.iter()) { // Verify the names match if new_field.name != old_field.name { return None; @@ -435,7 +436,7 @@ impl HotReloadResult { return None; } let literal = self.full_rebuild_state.hotreload_hot_literal(new_value)?; - literal_component_properties.push(literal); + literal_component_properties[*index] = Some(literal); } _ => { if new_field.value != old_field.value { @@ -445,7 +446,7 @@ impl HotReloadResult { } } - Some(literal_component_properties) + Some(literal_component_properties.into_iter().flatten().collect()) } /// Hot reload an if chain diff --git a/packages/rsx/tests/hotreload_pattern.rs b/packages/rsx/tests/hotreload_pattern.rs index 1ad9a3a15..c4b32a4af 100644 --- a/packages/rsx/tests/hotreload_pattern.rs +++ b/packages/rsx/tests/hotreload_pattern.rs @@ -1069,8 +1069,20 @@ fn component_with_handlers() { } }; - let valid = can_hotreload(a, b); - assert!(valid); + let hot_reload = hot_reload_from_tokens(a, b).unwrap(); + let template = hot_reload.get(&0).unwrap(); + assert_eq!( + template.component_values, + &[ + HotReloadLiteral::Int(456), + HotReloadLiteral::Float(789.456), + HotReloadLiteral::Bool(false), + HotReloadLiteral::Fmted(FmtedSegments::new(vec![ + FmtSegment::Literal { value: "goodbye " }, + FmtSegment::Dynamic { id: 0 } + ])), + ] + ); } #[test]