-
Notifications
You must be signed in to change notification settings - Fork 188
fix: add missing component macro documentation
#495
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
ealmloff
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the information on ReadOnlySignal in props and extending props is useful, but a lot of this is incorrect or duplicated from the state/reactivity guide, which makes the guides more difficult to maintain. The formatting also doesn't match the rest of the guide. The rest of the reference is paragraph style, but you use bullet points for half of your additions.
|
|
||
| - **`use_signal` vs. `use_memo`**: Use `use_signal` when the value will change due to internal component state or user interaction. Use `use_memo` when deriving a value from other reactive props or signals and you want automatic recomputation. | ||
|
|
||
| - **Signal updates are conditional on value changes**: If you call `.set()` on a signal with the same value it already holds, dependent effects and hooks will not re-run. If you need to force an update, consider mutating inner data if you're using something like `Signal<Vec<T>>`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't accurate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant by the first bullet point was the following:
#[component]
fn App() -> Element {
let mut count = use_signal(|| 0);
let double_count = use_memo(move || count() * 2);
rsx! {
div {
h1 { "Count: {count}" }
h2 { "Double Count (memoized): {double_count}" }
button {
onclick: move |_| {
count.set(count() + 1);
},
"Increment"
}
}
}
}In this example, count is a mutable signal. It's directly updated when the button is clicked (count.set()). double_count is derived from count. It's recomputed automatically whenever count changes.
By the second bullet point, I meant the following:
#[component]
fn App() -> Element {
let mut count = use_signal(|| 0);
let double_count = use_memo(move || {
count() * 2
});
rsx! {
div {
h1 { "Count: {count}" }
h2 { "Double Count (memoized): {double_count()}" }
button {
onclick: move |_| {
count.set(count() + 1);
},
"Increment"
}
button {
onclick: move |_| {
count.set(count());
},
"Set Same"
}
}
}
}In this example, the increment button updates count, so double_count will recompute. The "Set Same" button calls .set() with the same value count(), so no reactivity will be triggered. the double_count won't double. This demonstrates that signals only trigger updates if their value changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment was meant for line 220 (the second point)
The memo doesn't rerun its subscribers unless the value changes, but the signal does. If you click the set same button in this example, you can see the log as the memo reruns:
use dioxus::prelude::*;
fn main() {
dioxus::launch(app);
}
fn app() -> Element {
let mut count = use_signal(|| 0);
let double_count = use_memo(move || {
println!("Calculating double count");
count() * 2
});
rsx! {
div {
h1 { "Count: {count}" }
h2 { "Double Count (memoized): {double_count()}" }
button {
onclick: move |_| {
count.set(count() + 1);
},
"Increment"
}
button {
onclick: move |_| {
count.set(count());
},
"Set Same"
}
}
}
}Memos are already covered in the state guide. We don't need to duplicate that documentation here
|
|
||
| ```rust, no_run | ||
| rsx! { | ||
| div { "Value: {count()}" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This clones the value inside the signal instead of the pointer, which is much more expensive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant by this bullet point is that it's better to read the signal's value directly, rather than cloning it. For example:
div { "Value: {count()}" }Instead of:
div { "Value: {*(count.clone()).read()}" }There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends on the value inside the signal, but generally, the second code snippet is cheaper.
This will clone the value inside the signal, not return a reference to it:
div { "Value: {count()}" }This will clone the 32 byte signal and then return a reference to the inner value:
div { "Value: {*(count.clone()).read()}" }If your value is larger than 32 bytes, the second code snippet will save cloning that value. I also don't see how this fits into the managing props reference. If this were true, it would be better to document this in the state guide where signals are explained
| - **Avoid extending incompatible element types**: You must **only extend attribute sets that match the tag you're rendering**. Extending `input` attributes on a `div` may compile, but attributes like `type` or `value` will be meaningless or even silently dropped in the browser. | ||
|
|
||
| - **Prop filtering is manual**: If you want to restrict which attributes are allowed or filter out certain ones (e.g., to enforce accessibility or prevent accidental overrides), you must do this manually inside your component logic. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think these two statements add anything. What would the alternative behavior be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, in the first bullet point i was explaining what NOT to do:
#[component]
fn Bad(
// Extending input attributes on a <div> (not meaningful!)
#[props(extends = GlobalAttributes, extends = input)] attrs: Vec<Attribute>,
) -> Element {
rsx! {
div {
// Attributes like `type` or `value` are not on <div>.
..attrs,
}
}
}Instead, here is how you should do it correctly:
#[component]
fn Good(
// Extending input attributes on an <input>.
#[props(extends = GlobalAttributes, extends = input)] attrs: Vec<Attribute>,
) -> Element {
rsx! {
input {
..attrs,
}
}
}This means that element-specific attributes must be extended on matching elements. Otherwise, attributes like type and value have no semantic meaning on a <div>. Browsers won't interpret them in any special way. So, Dioxus doesn't prevent you from rendering type="text" on a <div>, but you shouldn't because:
- Browsers won't do anything with them on a
<div>. - For semantic HTML, it's incorrect and can confuse other developers (or screen readers, accessibility tools, etc.).
- Using
<div type="text">is not valid HTML (even though it renders).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The statements are clear enough, but I'm struggling to think of a situation where this information would be useful. If you try to add type: "text" to a div, what would you expect to happen other than the attribute getting ignored? I guess the behavior here that might not be expected is that spread attributes are not type-checked
- Prop filtering is manual: If you want to restrict which attributes are allowed or filter out certain ones (e.g., to enforce accessibility or prevent accidental overrides), you must do this manually inside your component logic.
I don't see any unexpected behavior here. If I have an API that returns Vec<usize>, it doesn't make sense to document that if you want even elements, you would need to manually filter the elements. That follows from the rust general docs.
|
|
||
| - **Spreading is shallow**: `..attrs` simply forwards attribute key/value pairs. If you need to merge structured data (e.g., two `style` maps), you must handle it manually. | ||
|
|
||
| - **Debugging tip**: When using `..attrs`, it can be hard to trace where an unexpected attribute came from. Consider logging or printing props during development to confirm which attributes were forwarded. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this adds anything. The logging guide explains logging and we can assume people have basic debugging skills
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What i meant by the logging bullet point is the following:
#[component]
fn App() -> Element {
rsx! {
// Accidentally passing an unrelated attribute "data-unknown" to Button
Button {
title: "Hello World",
class: "btn",
id: "main-button",
// Oops! This isn't intended
"data-unknown": "???"
}
}
}
#[component]
fn Button(
#[props(extends = GlobalAttributes, extends = button)] attrs: Vec<Attribute>,
) -> Element {
// Log attributes to see what was forwarded
tracing::info!("Button received attributes: {:?}", attrs);
rsx! {
button {
..attrs,
"Action!"
}
}
}When running the app, you can immediately see that data-unknown got forwarded. In a real-world app, this might hint that you're passing props that aren't actually supported by your component or the underlying DOM element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The statement is clear, but we don't need to document the fact that you can log things for every API. There isn't anything special about logging spread attributes, signals, events, etc. It is true, but it just bloats the documentation, which makes it more difficult to skim through, search, and maintain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comments and the overall review. I learned some new things about signals/memos that I wasn't aware of. I'll take the above points into consideration and work on improving the documentation as a whole.
Fixes #438