-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Added optional cleanup support to onmounted #5117
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
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 working on this! API looks a lot nicer, but I would like to avoid more thread locals and we may need deeper integration into core to track removals.
We would also need support for the webview renderer before this is merged. The code should be fairly similar once the web version is fully working
packages/html/src/events/mounted.rs
Outdated
| // Handler returns a cleanup closure | ||
| impl<F: FnOnce() + 'static> SpawnIfAsyncWithCleanup<CleanupMarker> for F { | ||
| fn spawn_with_cleanup(self) { | ||
| MOUNTED_CLEANUP.with(|c| *c.borrow_mut() = Some(Box::new(self))); |
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.
Instead of a thread local, can you add the cleanup callback to the mounted event? This variant would need to do something like event.data().set_on_cleanup(self)
packages/web/src/mutations.rs
Outdated
|
|
||
| // Invoke cleanup closure BEFORE replacing the element | ||
| #[cfg(feature = "mounted")] | ||
| self.invoke_cleanup(id); |
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'm not sure this will catch all removals. If we generate mutations efficiently, only the parent should be replaced when diffing from:
div {
button { onmounted }
}To:
p { "node removed" }The virtual dom knows which nodes are recycled, but it doesn't currently show up in mutations. We could add a new FreeId mutation which just drops the reference from the js heap instead of relying on the id being re-used later. That would also allow us to garbage collect dom nodes earlier in general
Implements ealmloff's PR DioxusLabs#5117 feedback: 1. Store cleanup on MountedData via event.data().set_on_cleanup(closure) 2. Add FreeId mutation emitted when element IDs are freed Changes: - Add FreeId variant to Mutation enum with #[non_exhaustive] - Add free_id() to WriteMutations with default no-op for compatibility - Add set_on_cleanup()/take_cleanup() methods to MountedData - Add ListenerCallback::new_raw() for custom event downcast logic - Add Event::with_data() to create events with different data - Implement cleanup invocation in web and desktop renderers - Accept both MountedData and PlatformEventData in onmounted handler Bbackward compatible for app developers. Custom renderers get default no-op for free_id().
|
@ealmloff thanks for the feedback and guidance on this. Please see this new checkin where I've tried to achieve what you requested but honestly, it was a step beyond my current depth of understanding of the Dioxus framework. That said, I needed "a little → a lot" of claude-code help. I came at it from different angles to test it and ensure it was backward compatible "for the app developer", but it needs eyes on it. Thanks for being kind if it's terrible.
It solves the problem for animation engine development I'm tinkering with. |
Hi @ealmloff, This PR implements the closure strategy discussed. It adds an optional cleanup closure to
onmountedhandlers. When an element is removed from the DOM, any cleanup closure returned by itsonmountedhandler is automatically invoked.Here is how it would be used:
Changes
onmountedhandlers can now return aFnOnce()cleanup closureremove,replace_with)()orasync {}continue to workTesting
Documentation