-
Notifications
You must be signed in to change notification settings - Fork 58
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
Have watchers track like computeds? #195
Comments
I just realized this also carries a unique benefit: nothing has to accept an API class except for the operand of the class itself. It's entirely implicit. This allows for polyfills to use private fields for everything. |
This API is somewhat less expressive than the current Watcher interface, if One could also imagine a (non-resetting)
It's slightly more complicated, but supports both styles and prevents you from having to make closures to wrap signals just to watch them. This does end up leaving out "unwatch a specific signal", so we'd want to double check the use cases there. Being able to batch calls to (I'm going to make a note that we should follow up on the watch/unwatch batching. We had some discussions about it early on, on a draft that had Effect rather than Watcher, and seemed to come to a consensus that "outside the outermost Computed or Effect" was lazy enough to batch and prompt enough about cleaning things up, but it's not the only coherent choice.) As for the implementation notes... I'm fairly skeptical of anything that involves trying to make direct state->watcher or watcher->state edges, because there are graph structures where this blows up the total number of edges (and the work to create and later traverse them) quadratically. What's the problem you're trying to solve here, or what is it that's unsatisfactory about just treating Watchers in the graph the same way as we would a Computed that nothing else depends on? |
@shaylew Apologies for the length.
This is a good point around not auto-resetting, and I'm okay with separating the two. Admittedly, my API suggestion here was a bit bold and audacious.
I could see that, though a separate
It sounds reasonable on the surface, but it can cause abstractions to inexplicably break. Consider this situation, where one watcher watches a signal, but that same signal is also accessed by a parent computed watched by a different watcher:
Now, you have two problems:
You need a clean boundary between watcher and parent computed, and the current proposed
Yeah, that batching is one of my main concerns, and it's part of what motivated me to suggest this as a means to delimit watchers, separate from
You can't really do watcher reactions without at least indirect state -> watcher references. Even the polyfill has them indirectly. Having each The state (or really, signal) -> watcher links are just for the batching mechanism. Other strategies are possible for it.
Could you elaborate? Not sure what you're talking about. |
On the algorithm:
It doesn't avoid the traversal, so much as it makes you do it eagerly at Notable to this situation is that, in the proposal's model:
You can basically think about these bounds on a per-edge basis: each tracked read or As far as I can tell, trying to maintain a Set on each State doesn't play nice with those bounds. You pay for n^2 links, every consecutive |
@shaylew Okay, I took a step back, to focus more on the problem at hand. What about either of these possible solutions, using a Here's what I'm thinking at a high level (skipping around the value setting/propagating for brevity):
|
Okay, I came up with a simpler alternative that punts the whole question to userland, while still preventing double-notification: #222 For several promises, you can either aggregate them via |
It'd simplify watcher management a lot, and it'd make it work like a mirror image of
untrack
. It also would simplify implementations somewhat and mitigate some perf hazards aroundSignal.subtle.{,un}watch
.The idea is this:
watcher.track(body)
: Tracks any newly accessed signals inbody()
, and afterbody()
returns/throws, removes any signals not accessed. Returns the result ofbody()
's evaluation. The idea is it mirrorsuntrack(body)
.watcher.reset()
: Equivalent towatcher.track(() => {})
, useful for clearing all signals in the watcher.To add to a watcher, simply
.get()
inside atrack
ed callback. And to remove, simply omit.This would also remove the need for intermediate
Computed
s for such auto-tracking.This stands somewhat in opposition to some of what I've suggested in #178, in that the watcher would have to retain a list of signals it depends on. However, it'd simplify the internal execution model a bit:
Watcher
s track dependentState
s.Computed
s track dependentComputed
s andState
s.get
, add the signal to the currentWatcher
if not already present.Computed#get
, it recurses through its dependencies to wire everything up.State
dependency, add theState
to the currentWatcher
if not already present. If dirty, treat the parentComputed
as dirty.Computed
dependency, add theComputed
to the currentWatcher
if not already present, and recurse into its children. If after that, the dependencyComputed
is treated as dirty already, treat the parentComputed
as dirty.Computed
's dependencies treat it as dirty, toss the children (removing descendantState
s from the currentWatcher
and vice versa if necessary) and execute theComputed
's body. If the result is equal, treat the parentComputed
as not dirty. Speculative descent can speed this up considerably, at cost to some memory churn (it may be worth putting this into the spec as well):Watcher
's dependentState
set and let this be the saved set.Computed
to be dirty, return.Watcher
's dependentState
set with the saved set. Now, the speculative set is the one saved and the old set is the one being added to.Computed
's body. Treat thisComputed
's parent as dirty if and only if the result equals the previous result.State
in the saved (speculative) set not in the (possibly updated) currentWatcher
's dependentState
set, remove the currentWatcher
from thatState
's watcher set.State
set and set it to an empty set..track(...)
as the current set isn't accessed here..track(...)
as the current set isn't accessed here.The text was updated successfully, but these errors were encountered: