-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Hook order restriction #2026
Comments
Looking at this a bit more, I have realised that the panic isn't even the worst part of this ordering bug. If you use conditions with hooks in a particular way one hook can start reading and writing to another state by accident if they are the same type. Here is a pretty contrived example: #[function_component(Counter)]
pub fn counter() -> Html {
let count = use_ref(|| 0);
let rest = {
let count = count.clone();
move |state: UseStateHandle<usize>, action: fn(usize) -> usize| {
let onclick = {
let count = count.clone();
let state = state.clone();
move |_| {
*count.borrow_mut() += 1;
state.set(action(*state));
}
};
html! {
<div>
<button {onclick}>{ "Count" }</button>
<br />
{ format!("count: {}", *count.borrow()) }
<br />
{ format!("state count: {}", *state) }
</div>
}
}
};
if *count.borrow() % 2 == 0 {
rest(use_state(|| 0), add_1)
} else {
rest(use_state(|| 100), add_50)
}
}
fn add_1(i: usize) -> usize {
i + 1
}
fn add_50(i: usize) -> usize {
i + 50
} So if working correctly "state count" would display the following when the button was clicked: 0, 100, 1, 150, etc. The example does show that you have twist yourself in knots to get this to happen but I can see it happening out in the wild and unlike the panic this bug is insidious and could be a real headache to debug. |
Unfortunately, I don't think this would work because there are plenty of edge cases. What if, for example, you use a hook inside a loop? Or calling a function that uses hooks? In fact, this will completely break the composability aspect of hooks because custom-hooks will call the primitive hooks at the same location in the source file. |
If you put a hook inside a loop using this technique I actually think it would work - it would call the runner for the same hook state as many times as the loop, now whether a user of the hook would think the hook should actually be unique for each invocation of the loop I don't know so this could probably lead to confusion?
This is the breaking point for this approach because using line/column/file only works if you call those at the call site of the hook in the function component that means anything else breaks down completely and again we have no way to stop this being abused. I agree that this would not be the way to fix this issue, but I'm not really sure what could be the fix here and I really don't like that hooks can have this behaviour - it's good enough for JS to say "don't do this" and blow up if you do but it feels very wrong in Rust. |
Problem
Hooks must be executed in the same order otherwise the function component will panic when trying to convert the hook from hook state for the wrong hook type.
This was mentioned in #1129 and deserves it's own issue.
I have considered this a bug because it's a runtime panic and there are no guards against a user doing this by accident.
Steps To Reproduce
Minimal Code to reproduce panic:
Load this app up in the browser, it will render once without issue but once you click the "Count" button it will panic:
panicked at 'Incompatible hook type. Hooks must always be called in the same order',
Not very useful even if it did work correctly but keeps it as simple as possible to cause the panic.
Expected behavior
To either fail at compile time when trying to use a hook conditionally or for hooks to not depend on the order in which they run to work correctly.
Environment:
master
As mentioned in #1129 a possible solution is to macro-fy the hook API so that in the macro we can sneak in calls toline!
/column!
/file!
.Edit: The possible solution above would not solve this issue.
The text was updated successfully, but these errors were encountered: