Skip to content
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

ImmediateEffect #3650

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

QuartzLibrary
Copy link

@QuartzLibrary QuartzLibrary commented Feb 24, 2025

This introduces ImmediateEffect, which executes its effect once upon creation and then every time a dependency changes, immediately.

This is not what you want in most code, but is useful for some kinds of double-binding (for example to some syncing mechanism like CRDTs, or a signal to the browser's route state).

I think this can live outside leptos, so if you think it's better to keep it out or this version is not quite where you want it, I am happy to close this.


A note on Send and Sync: since the dependency that triggers the effect could be in any thread, this also means the effect always has to be Send and Sync. I still matched the Effect interface.
This is unfortunate, but I couldn't figure out a way to avoid that without unsafe, and even then it would have 'solved' the problem by panicking if called from another thread.

I left that code in an intermediate commit, but as there is currently no unsafe code in leptos, I don't think it's worth it even if it's sound (as a casual observer would lose the simple and strong no-unsafe anywhere guarantee).


cc: #2822

@QuartzLibrary QuartzLibrary changed the title Immediate effect ImmediateEffect Feb 24, 2025
Copy link
Collaborator

@gbj gbj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thank you! This would close #2822.

I have looked over the implementation and it certainly looks correct to me. The only thing missing is adding some tests -- my assumption would be that copying some of the tests in tests/effects.rs, using immediate effects, and dropping the various Executor::tick().await; points (which wait for the current, batched/async effects to run) would be sufficient to test this!

Really nice work, and much appreciated.

@gbj
Copy link
Collaborator

gbj commented Feb 25, 2025

(Oh, and just to add a note on the Send/Sync bounds: these are okay, and definitely a requirement. I'd say that for this niche use case, I'm perfectly happy to say that users who need to run them on the current thread are very likely doing so in a single-threaded browser context and can use SendWrapper to wrap a !Send effect, etc.)

@QuartzLibrary
Copy link
Author

The only thing missing is adding some tests

Definitely! I wanted to get it up as soon as possible to avoid doing this in the dark.

So, some more design considerations:

  • If we allow recursion, we have to use Fns.
    I'm fine with using Fns because it's a more general interface and this is a bit of a 'power user' feature anyway. I have a helper that simulates panic-on-recursion usage, but I'm leaning toward removing it before merging. Skip-inner-recursion is also doable.
  • If we allow recursion, then does it make sense to pass in the Option<T> to the effect function? On recursion it would be always None as the old value is still being used by the level above. I'm leaning toward no. The user can pass in additional state if they want/need it.
  • If we allow recursion, the sources situations is a bit tricky, as in normal effects we clear them before every run. That might be unexpected under recursion. So, options:
    • As is: clear before every run, keep the last run in full and whatever comes after that in the parent runs.
    • Keep it all: track all sources from all recursion levels (clear the sources only before the top-level run).
    • Keep only the last run: figure out what the last run is, and only track things from that, ignoring any tracking that technically comes after it from the parents.

I think I am leaning toward 'allow recursion', no Option<T>, and keeping only the last run, but I'll sit on it a bit and see if that changes.

@QuartzLibrary
Copy link
Author

Implemented it as described above, I'll probably give it a look over the weekend with fresh eyes. It'll also need some multi-threaded tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants