-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Mutate Observer #16183
base: main
Are you sure you want to change the base?
Mutate Observer #16183
Conversation
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
5cb7832
to
61511de
Compare
You added a new example but didn't add metadata for it. Please update the root Cargo.toml file. |
9ca79c0
to
66db6df
Compare
You added a new example but didn't add metadata for it. Please update the root Cargo.toml file. |
66db6df
to
4d1e9cb
Compare
You added a new example but didn't add metadata for it. Please update the root Cargo.toml file. |
4d1e9cb
to
7f1357d
Compare
You added a new example but didn't add metadata for it. Please update the root Cargo.toml file. |
0e4cdd7
to
5933212
Compare
You added a new example but didn't add metadata for it. Please update the root Cargo.toml file. |
f836647
to
4888eec
Compare
You added a new example but didn't add metadata for it. Please update the root Cargo.toml file. |
313690e
to
13bdcf9
Compare
The generated |
13bdcf9
to
6d9d23d
Compare
The generated |
6d9d23d
to
7d6996d
Compare
7d6996d
to
1175027
Compare
1175027
to
ea62087
Compare
ea62087
to
c98f097
Compare
@alice-i-cecile Hi alice, i thought this pr is ready to review now. What should I do next? |
@pyhinox there are some merge conflicts, but I'll go ahead and mark it for review to start getting eyes on it. 🙂 |
Could you use CI is failing for a few invalid links in documentation, mostly things that are not in scope for automatic doc links |
I thought we had a lint enabled to catch that as well. Good eye. |
I think that we can use #16208 to remove the need for this. If certain components can't be mutated, we can do all of our reactive logic in the insertion / removal stage 🎉 |
I don't believe clippy lints apply to documentation sadly. I got bit by that when adding the lints! |
@alice-i-cecile how does #16208 remove the need for this? What happens if you want to observe a component changing? |
I believe the idea was if you want to watch a component, make that component immutable so that the current hooks can capture all state changes. It doesn't cover the case where you want to observe a component which isn't immutable though. |
Precisely.
Yeah. Ultimately, virtually all of the same drawbacks apply to both the OnMutate and the immutable components approaches to this problem:
I'd like to see if we can get away with only using a single new concept to achieve these benefits, and then re-evaluate once we have a clearer picture of the final performance and ergonomics. EDIT: on reflection, I think that this is probably still useful, because it allows users to do reactivity things with any arbitrary component, which is really valuable for UI. I suspect that we should have on-mutate observers fill a "not very strict + deferred reactions" niche, while reserving immutable components + hooks for really critical invariants that must be constantly upheld. |
Yeah I'm starting to want something like this as part of our reactivity solution, over less event driven / poll based approaches (tbd though). But yeah the current unconditional impl should not be merged. If we land this, we can't affect the default Mut path. That is hot / costly enough as it is. The |
Objective
OnMutate
observer and demonstrate how to use it for UI reactivity #14520 and adds OnMutate observers.Solution
A
EntityChanges
which stores the change list is added toWorld
. WhenMut::set_changed
is called, a change record will be added to theEntityChanges
.When
World::flush
is called,EntityChanges
will be clear and triggerOnMutate
event to each relative entity.Example
Testing
Just test for the base function, I don't know which test i should do.
Query::iter
andQuery::get_mut
's benchmarkJust treat the
OnMutate
as a common observer. Nothing is specific.Mac
Possible Follow-ups
OnMutate
hook