-
-
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
Introduce a dedicated use_force_update hook #2586
Conversation
Visit the preview URL for this PR (updated for commit a8043fd): https://yew-rs--pr2586-use-rerender-icb12nwz.web.app (expires Thu, 14 Apr 2022 16:16:25 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
Size Comparison
|
This hook is consistently faster (x 6-7) on my Laptop than the alternative of using state. #[function_component]
fn BenchSetState() -> Html {
let state = use_state(|| ());
use_effect_with_deps(
move |_| {
let start_time = instant::now();
for _ in 0..1_000_000 {
state.set(());
}
log!(format!(
"BenchSetState: Scheduled 1 million renders in {}ms.",
instant::now() - start_time
));
|| {}
},
(),
);
Html::default()
}
#[function_component]
fn BenchForceUpdate() -> Html {
let state = use_force_update();
use_effect_with_deps(
move |_| {
let start_time = instant::now();
for _ in 0..1_000_000 {
state.force_update();
}
log!(format!(
"BenchForceUpdate: Scheduled 1 million renders in {}ms.",
instant::now() - start_time
));
|| {}
},
(),
);
Html::default()
} Firefox:
Chrome:
|
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 pull request resolves the bigger question raised in the issue (why they cannot use #[hook]
to create a hook). Which is an issue that I think should be investigated before v0.20.
In addition, even with the performance improvement, I don't think it's good to have a hook to encourage rendering without changing any state.
If a function component's rendered layout is purely determined by props
+ context
+ state
s, all of them will automatically subscribe for a re-render upon changes.
This is true even if Yewdux
or Bounce
is used, which an Rc'ed copy of the state will be produced and set into a Yew-managed state.
If you have a state not managed by Yew (or interior mutability), there's no guarantee that your state will stay the same until the time a render actually happens (as render is delayed), which results in inconsistency in rendered result.
This situation will only get worse if more concurrent mode features are introduced.
Which is true as long as you don't use internally mutable state or state that is not managed by Yew. In such a situation, rather than introducing fake state I can't fathom how many useless clones of data is made under the immutable model that react pushes to protect developers from themselves ("never use mutable structure", "use spread syntax", etc). And to some degree the solution is libraries offering cheaply cloneable, immutable, i.e. persistent data structures in rust (I see the PR for that). But you won't be able to retrofit libraries to that model, and to really push performance, at some point mutable data just is faster. (I think the theoretical bound was some kind of
Consistency is even more of a problem if I start cloning the state all over instead of relying on a single-source of truth. Matter of fact, as long as the It's a fundamental part of the model that at some point a re-render is triggered. Just embrace it and offer a hook for it, with a few warning labels attached. The standard rust programmer is different from the standard javascript programmer and exposing some more low level primitives shouldn't hurt. Maybe I would add a warning that the re-render isn't guaranteed to always run immediately? In any case, I don't quite see how concurrent mode will mess with something as fundamental as triggering a re-render.
I also don't want to encourage it, that's why the warnings are there and all the docs tell you to prefer something else. But I want it to be there if someone really needs it without sacrificing performance. Maybe keep it out of the website or move it somewhere under the "Advanced topics" section. |
Btw, the |
One place I could see this hook being very useful is if I want to have a context without wrapping it in a reducer. For example: struct Context {
data: RefCell<String> // or something else, this type doesn't matter,
re_render: UseForceUpdate
}
impl Context {
fn data(&self) -> &String { &*self.data.borrow() }
fn set_data(&self, value: String) {
*self.data.borrow_mut() = value
self.re_render.force_update()
}
} This is a lot less boilerplate-y compared to a solution which uses a reducer. This goes together with my comment #2563 (comment):
Rust offers us mutability (interior mutability is part of it) and we should embrace it instead of trying to bend over to offer a JS-way to solve a problem |
How do you do it without securing it with expected state that is guaranteed to show as expected after the next time its written to?
No, The answer is in the link you posted and the update version: reactwg/react-18#86
Rust has a nature to make discouraged practice "difficult to use" with |
I don't see how any of those are "difficult" to use, seems pretty straight forward.
Agreed.
Ah sure, seems a bit easier to use. And totally increases my trust in their design process. But my main concern with this PR is not to allow reading from mutable source, but just offer a bail-out mechanism. This is not concerned with Concurrent Mode or Tearing (which is more-or-less a problem with not being able to identify rendering context without the additional API), which should be solved when it comes up. This only triggers an unconditional rerender, for any reason and I expect it to be safe, although maybe causing slightly too many re-renders once CM exists and we can think about more high-level API for that. But semantically, calling Like I asked before: would it be reasonable to move the website documentation to give the hook less exposure to beginners? |
The main reason you want to do this is you are trying to read from states not synchronised with yew's rendering cycle. FWIW, https://docs.rs/yew-hooks/latest/yew_hooks/fn.use_update.html If this is discouraged, and you need to schedule many many renders (very unoptimised) before a render happens to make any realistic difference, is there any good reason to make it available in |
Yes, the fact that if it's used, at least it's reasonably efficient. And people will (and apparently already have) hack their way around it if it doesn't exist. |
The fact that Yew is a Rust library makes things different. Rust is about giving control. If we can give control, we should. However we should also have general API that are to be used in vast majority of cases |
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 think we can go ahead and merge this
// #![feature(fn_traits)] // required nightly feature to make UseForceUpdate callable directly | ||
// impl Fn<()> for UseForceUpdate { | ||
// extern "rust-call" fn call(&self, _args: ()) { | ||
// self.force_update() | ||
// } | ||
// } |
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.
We should put this behind a nightly
feature flag but that can be done in a separate PR
/// A handle which can be used to force a re-render of the associated | ||
/// function component. | ||
#[derive(Clone)] | ||
pub struct UseForceUpdate { |
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.
Should we follow UseXXXHandle
convention and rename it to UseForceUpdateHandle
? like UseReducerHandle
/ UseStateHandle
etc
From |
I originally planned to discuss this topic when I have time to implement There is an important distinction between:
Blocking means that an update must happen on the spot, regardless of whether the browser is busy or not. Interior mutability means choosing the first option, which seems fast, but at the cost of blocking the browser until everything is settled. Concurrent mode, on the other hand, elects the second option, for updates that can be delayed, it updates in a copy of the state when the browser is idle. This means that multiple copies of a state will exist "concurrently". However, this means that if in the meanwhile an urgent update happens, it will "rollback" instead of "commit". Interior Mutability means we cannot "rollback" when a transition becomes outdated before committing. Extensively using interior mutability means that your state always need to be committed in "urgent" mode so that later updates will be able to see previous updates, which blocks the browser. I think it may be fine to merge this at the moment, but we might need to revisit the design of this hook in the future (by making it into a hook that marks the current render as "urgent"?). |
We don't have concurrent mode yet (to be honest, I don't even understand what it is and what problem it solves). If we plan on landing concurrent mode before 1.0, we can make a breaking change if needed. I'll merge this now as it (and any other APIs) can be updated before a stable release as needed. @futursolo it would be nice if you could create a discussion/issue about concurrent mode, explaining what it is, why it's needed and what problem it solves. That would help those (myself included) who haven't used it in other ecosystems, such React. |
Description
Introduce a hook that can unconditionally trigger a rerender. Avoids an idiosyncratic use of
use_reducer
oruse_state
and introduces a few less allocations.Targets, but doesn't complete #2576 (closes the last api gap)
Checklist
cargo make pr-flow