diff --git a/Cargo.lock b/Cargo.lock index ce5ab6c12..cb10acc37 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2526,6 +2526,7 @@ dependencies = [ "tracing", "tracing-fluent-assertions", "tracing-subscriber", + "warnings", "web-sys", ] @@ -2711,6 +2712,7 @@ dependencies = [ "slab", "tokio", "tracing", + "warnings", "web-sys", ] @@ -2970,6 +2972,7 @@ dependencies = [ "tokio", "tracing", "tracing-subscriber", + "warnings", ] [[package]] @@ -10363,6 +10366,25 @@ dependencies = [ "try-lock", ] +[[package]] +name = "warnings" +version = "0.1.0" +source = "git+https://github.com/DioxusLabs/warnings#9889b96cccb6ac91a8af924cfee51a8895146d08" +dependencies = [ + "pin-project", + "warnings-macro", +] + +[[package]] +name = "warnings-macro" +version = "0.1.0" +source = "git+https://github.com/DioxusLabs/warnings#9889b96cccb6ac91a8af924cfee51a8895146d08" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.71", +] + [[package]] name = "wasi" version = "0.9.0+wasi-snapshot-preview1" diff --git a/packages/core/Cargo.toml b/packages/core/Cargo.toml index ca6cb0954..696c64f26 100644 --- a/packages/core/Cargo.toml +++ b/packages/core/Cargo.toml @@ -24,6 +24,7 @@ serde = { version = "1", features = ["derive"], optional = true } generational-box = { workspace = true } rustversion = "1.0.17" const_format = { workspace = true } +warnings = { git = "https://github.com/DioxusLabs/warnings" } [dev-dependencies] tokio = { workspace = true, features = ["full"] } diff --git a/packages/core/src/properties.rs b/packages/core/src/properties.rs index e419e3864..b7d201c07 100644 --- a/packages/core/src/properties.rs +++ b/packages/core/src/properties.rs @@ -117,34 +117,36 @@ where P::builder() } +/// A warning that will trigger if a component is called as a function +#[warnings::warning] +pub(crate) fn component_called_as_function, P, M>(_: C) { + // We trim WithOwner from the end of the type name for component with a builder that include a special owner which may not match the function name directly + let mut type_name = std::any::type_name::(); + if let Some((_, after_colons)) = type_name.rsplit_once("::") { + type_name = after_colons; + } + let component_name = Runtime::with(|rt| { + current_scope_id() + .ok() + .and_then(|id| rt.get_state(id).map(|scope| scope.name)) + }) + .ok() + .flatten(); + + // If we are in a component, and the type name is the same as the active component name, then we can just return + if component_name == Some(type_name) { + return; + } + + // Otherwise the component was called like a function, so we should log an error + tracing::error!("It looks like you called the component {type_name} like a function instead of a component. Components should be called with braces like `{type_name} {{ prop: value }}` instead of as a function"); +} + /// Make sure that this component is currently running as a component, not a function call #[doc(hidden)] -#[allow(unused)] +#[allow(clippy::no_effect)] pub fn verify_component_called_as_component, P, M>(component: C) { - #[cfg(debug_assertions)] - { - // We trim WithOwner from the end of the type name for component with a builder that include a special owner which may not match the function name directly - let mut type_name = std::any::type_name::(); - if let Some((_, after_colons)) = type_name.rsplit_once("::") { - type_name = after_colons; - } - let component_name = Runtime::with(|rt| { - current_scope_id() - .ok() - .and_then(|id| rt.get_state(id)) - .map(|scope| scope.name) - }) - .ok() - .flatten(); - - // If we are in a component, and the type name is the same as the active component name, then we can just return - if component_name == Some(type_name) { - return; - } - - // Otherwise the component was called like a function, so we should log an error - tracing::error!("It looks like you called the component {type_name} like a function instead of a component. Components should be called with braces like `{type_name} {{ prop: value }}` instead of as a function"); - } + component_called_as_function(component); } /// Any component that implements the `ComponentFn` trait can be used as a component. diff --git a/packages/core/src/virtual_dom.rs b/packages/core/src/virtual_dom.rs index 2b1709232..d9dc420c0 100644 --- a/packages/core/src/virtual_dom.rs +++ b/packages/core/src/virtual_dom.rs @@ -254,8 +254,15 @@ impl VirtualDom { /// ``` /// /// Note: the VirtualDom is not progressed, you must either "run_with_deadline" or use "rebuild" to progress it. - pub fn new Element + Clone + 'static>(app: F) -> Self { - Self::new_with_props(app, ()) + pub fn new(app: fn() -> Element) -> Self { + Self::new_with_props( + move || { + use warnings::Warning; + // The root props don't come from a vcomponent so we need to manually rerun them sometimes + crate::properties::component_called_as_function::allow(app) + }, + (), + ) } /// Create a new VirtualDom with the given properties for the root component. @@ -313,7 +320,7 @@ impl VirtualDom { } /// Create a new virtualdom and build it immediately - pub fn prebuilt Element + Clone + 'static>(app: F) -> Self { + pub fn prebuilt(app: fn() -> Element) -> Self { let mut dom = Self::new(app); dom.rebuild_in_place(); dom diff --git a/packages/hooks/Cargo.toml b/packages/hooks/Cargo.toml index ec42fe395..5820173ee 100644 --- a/packages/hooks/Cargo.toml +++ b/packages/hooks/Cargo.toml @@ -22,6 +22,7 @@ slab = { workspace = true } futures-util = { workspace = true} generational-box.workspace = true rustversion = "1.0.17" +warnings = { git = "https://github.com/DioxusLabs/warnings" } [dev-dependencies] futures-util = { workspace = true, default-features = false } diff --git a/packages/hooks/src/use_coroutine.rs b/packages/hooks/src/use_coroutine.rs index abbf7e627..347ec38fd 100644 --- a/packages/hooks/src/use_coroutine.rs +++ b/packages/hooks/src/use_coroutine.rs @@ -1,3 +1,4 @@ +use ::warnings::Warning; use dioxus_core::prelude::{consume_context, provide_context, spawn, use_hook}; use dioxus_core::Task; use dioxus_signals::*; @@ -84,13 +85,17 @@ where // We do this here so we can capture data with FnOnce // this might not be the best API - if *coroutine.needs_regen.peek() { - let (tx, rx) = futures_channel::mpsc::unbounded(); - let task = spawn(init(rx)); - coroutine.tx.set(Some(tx)); - coroutine.task.set(Some(task)); - coroutine.needs_regen.set(false); - } + dioxus_signals::warnings::signal_read_and_write_in_reactive_scope::allow(|| { + dioxus_signals::warnings::signal_write_in_component_body::allow(|| { + if *coroutine.needs_regen.peek() { + let (tx, rx) = futures_channel::mpsc::unbounded(); + let task = spawn(init(rx)); + coroutine.tx.set(Some(tx)); + coroutine.task.set(Some(task)); + coroutine.needs_regen.set(false); + } + }) + }); coroutine } diff --git a/packages/hooks/src/use_reactive.rs b/packages/hooks/src/use_reactive.rs index 67eebc287..d49881a6f 100644 --- a/packages/hooks/src/use_reactive.rs +++ b/packages/hooks/src/use_reactive.rs @@ -110,7 +110,14 @@ pub fn use_reactive( non_reactive_data.out() }); if !first_run && non_reactive_data.changed(&*last_state.peek()) { - last_state.set(non_reactive_data.out()); + use warnings::Warning; + // In use_reactive we do read and write to a signal during rendering to bridge the reactive and non-reactive worlds. + // We ignore + dioxus_signals::warnings::signal_read_and_write_in_reactive_scope::allow(|| { + dioxus_signals::warnings::signal_write_in_component_body::allow(|| { + last_state.set(non_reactive_data.out()) + }) + }); } move || closure(last_state()) } diff --git a/packages/signals/Cargo.toml b/packages/signals/Cargo.toml index 71d252816..977c59467 100644 --- a/packages/signals/Cargo.toml +++ b/packages/signals/Cargo.toml @@ -22,6 +22,7 @@ once_cell = "1.18.0" rustc-hash = { workspace = true } futures-channel = { workspace = true } futures-util = { workspace = true } +warnings = { git = "https://github.com/DioxusLabs/warnings" } [dev-dependencies] dioxus = { workspace = true } diff --git a/packages/signals/src/read_only_signal.rs b/packages/signals/src/read_only_signal.rs index 4be2116bc..0794d3f21 100644 --- a/packages/signals/src/read_only_signal.rs +++ b/packages/signals/src/read_only_signal.rs @@ -46,7 +46,13 @@ impl>> ReadOnlySignal { /// This should only be used by the `rsx!` macro. pub fn __set(&mut self, value: T) { use crate::write::Writable; - self.inner.set(value); + use warnings::Warning; + // This is only called when converting T -> ReadOnlySignal which will not cause loops + crate::warnings::signal_write_in_component_body::allow(|| { + crate::warnings::signal_read_and_write_in_reactive_scope::allow(|| { + self.inner.set(value); + }); + }); } #[doc(hidden)] diff --git a/packages/signals/src/signal.rs b/packages/signals/src/signal.rs index 301822cc6..6233cc393 100644 --- a/packages/signals/src/signal.rs +++ b/packages/signals/src/signal.rs @@ -603,39 +603,56 @@ struct SignalSubscriberDrop>> { origin: &'static std::panic::Location<'static>, } -impl>> Drop for SignalSubscriberDrop { - fn drop(&mut self) { - #[cfg(debug_assertions)] - { - tracing::trace!( - "Write on signal at {} finished, updating subscribers", - self.origin - ); +pub mod warnings { + //! Warnings that can be triggered by suspicious usage of signals - // Check if the write happened during a render. If it did, we should warn the user that this is generally a bad practice. - if dioxus_core::vdom_is_rendering() { - tracing::warn!( - "Write on signal at {} happened while a component was running. Writing to signals during a render can cause infinite rerenders when you read the same signal in the component. Consider writing to the signal in an effect, future, or event handler if possible.", - self.origin - ); - } + use super::*; + use ::warnings::warning; - // Check if the write happened during a scope that the signal is also subscribed to. If it did, this will probably cause an infinite loop. - if let Some(reactive_context) = ReactiveContext::current() { - if let Ok(inner) = self.signal.inner.try_read() { - if let Ok(subscribers) = inner.subscribers.lock() { - for subscriber in subscribers.iter() { - if reactive_context == *subscriber { - let origin = self.origin; - tracing::warn!( - "Write on signal at {origin} finished in {reactive_context} which is also subscribed to the signal. This will likely cause an infinite loop. When the write finishes, {reactive_context} will rerun which may cause the write to be rerun again.\nHINT:\n{SIGNAL_READ_WRITE_SAME_SCOPE_HELP}", - ); - } + /// Check if the write happened during a render. If it did, warn the user that this is generally a bad practice. + #[warning] + pub fn signal_write_in_component_body(origin: &'static std::panic::Location<'static>) { + // Check if the write happened during a render. If it did, we should warn the user that this is generally a bad practice. + if dioxus_core::vdom_is_rendering() { + tracing::warn!( + "Write on signal at {} happened while a component was running. Writing to signals during a render can cause infinite rerenders when you read the same signal in the component. Consider writing to the signal in an effect, future, or event handler if possible.", + origin + ); + } + } + + /// Check if the write happened during a scope that the signal is also subscribed to. If it did, trigger a warning because it will likely cause an infinite loop. + #[warning] + pub fn signal_read_and_write_in_reactive_scope>>( + origin: &'static std::panic::Location<'static>, + signal: Signal, + ) { + // Check if the write happened during a scope that the signal is also subscribed to. If it did, this will probably cause an infinite loop. + if let Some(reactive_context) = ReactiveContext::current() { + if let Ok(inner) = signal.inner.try_read() { + if let Ok(subscribers) = inner.subscribers.lock() { + for subscriber in subscribers.iter() { + if reactive_context == *subscriber { + tracing::warn!( + "Write on signal at {origin} finished in {reactive_context} which is also subscribed to the signal. This will likely cause an infinite loop. When the write finishes, {reactive_context} will rerun which may cause the write to be rerun again.\nHINT:\n{SIGNAL_READ_WRITE_SAME_SCOPE_HELP}", + ); } } } } } + } +} + +#[allow(clippy::no_effect)] +impl>> Drop for SignalSubscriberDrop { + fn drop(&mut self) { + tracing::trace!( + "Write on signal at {} finished, updating subscribers", + self.origin + ); + warnings::signal_write_in_component_body(self.origin); + warnings::signal_read_and_write_in_reactive_scope::(self.origin, self.signal); self.signal.update_subscribers(); } }