tc39 / proposal-signals

A proposal to add signals to JavaScript.
MIT License
2.87k stars 54 forks source link

Have watchers track like computeds? #195

Open dead-claudia opened 3 weeks ago

dead-claudia commented 3 weeks ago

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 around Signal.subtle.{,un}watch.

The idea is this:

To add to a watcher, simply .get() inside a tracked callback. And to remove, simply omit.

This would also remove the need for intermediate Computeds 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:

dead-claudia commented 3 weeks ago

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.

shaylew commented 3 weeks ago

This API is somewhat less expressive than the current Watcher interface, if track(fn) always resets -- it keeps you from being able to incrementally add things to a Watcher without repeating its previous (potentially long) list of dependencies. But if it doesn't reset it's definitely a nicer API, and worth floating.

One could also imagine a (non-resetting) track(fnOrSignal) that goes either way, and does:

  1. If typeof fnOrSignal is function, run it in the watcher's tracking context
  2. if fnOrSignal has a get property that's a function, run fnOrSignal.get() in the watcher's tracking context.

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 watch and unwatch is very useful, and I think we do want to figure out a good mechanism for that. One option would be to defer them until you're outside of the outermost Computed's .get, which seems reasonable as a baseline and (maybe less reasonably) lets you use a dummy computed with an untrack inside to force batching. Watcher track as a way to get batching seems just as reasonable.

(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?

dead-claudia commented 2 weeks ago

@shaylew Apologies for the length.

This API is somewhat less expressive than the current Watcher interface, if track(fn) always resets -- it keeps you from being able to incrementally add things to a Watcher without repeating its previous (potentially long) list of dependencies. But if it doesn't reset it's definitely a nicer API, and worth floating.

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.

One could also imagine a (non-resetting) track(fnOrSignal) that goes either way, and does:

  1. If typeof fnOrSignal is function, run it in the watcher's tracking context
  2. if fnOrSignal has a get property that's a function, run fnOrSignal.get() in the watcher's tracking context.

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.

I could see that, though a separate .watch method would make more sense than combining the two methods.

Being able to batch calls to watch and unwatch is very useful, and I think we do want to figure out a good mechanism for that. One option would be to defer them until you're outside of the outermost Computed's .get, which seems reasonable as a baseline and (maybe less reasonably) lets you use a dummy computed with an untrack inside to force batching. Watcher track as a way to get batching seems just as reasonable.

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:

  1. The component library starts inexplicably throwing. The end developer tracks it down pretty easily to the library and files a bug report against it. That library's framework documents the throwing function as safe in that given instant, so the library developer files a bug against the framework. The framework developer can't reproduce the issue since they don't know about the integration issue, and so they close it. End developer has no idea it's actually the other framework causing it, and is now raging about the whole situation on Twitter/X.
  2. During animations, the library component sets an inner signal's value. This causes the parent framework to recompute the tree. The tree is the same as before, since nothing functionally changed, but it takes long enough to cause occasional but frequent stutters on mobile. The end developer tracks this down, profiles their app, and is utterly baffled that their framework's even rendering at that time. They don't know about Signal.subtle.untrack or the fact the inner component's using signals, so they spend a few days straight, only to give up on figuring out what's causing the stutters.

You need a clean boundary between watcher and parent computed, and the current proposed .watch(...) API doesn't afford that.

(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.)

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 computed.get().

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.

You can't really do watcher reactions without at least indirect state -> watcher references. Even the polyfill has them indirectly.

Having each State hold a Set<Watcher> does allow you to entirely avoid traversing any graphs, though. With that, all you'd be doing is iterating that set each state.set(value).

The state (or really, signal) -> watcher links are just for the batching mechanism. Other strategies are possible for it.

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?

Could you elaborate? Not sure what you're talking about.

shaylew commented 5 days ago

On the algorithm:

You can't really do watcher reactions without at least indirect state -> watcher references. Even the polyfill has them indirectly.

Having each State hold a Set does allow you to entirely avoid traversing any graphs, though. With that, all you'd be doing is iterating that set each state.set(value).

It doesn't avoid the traversal, so much as it makes you do it eagerly at watch time, rather than lazily at set-time. This isn't obviously a bad trade, but I think it ends up being non-obviously a bad trade. In full (or mostly) generality you have some sort of graph like this:

mermaid-diagram-2024-05-13-014143

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 .watch(x) creates one edge, dirtying traverses each forward edge at most once until the nodes are cleaned, and watching/unwatching nodes traverses each backwards edge at most once until the nodes change to unwatched/watched again.

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 set to one of the Si has to visit all n watchers (even though they'll already be dirty and won't be notified again after the first set), and every watch or unwatch on the watchers will have to visit n states and add/remove n links (even if those states don't need to fire any callbacks if their overall liveness hasn't changed).

dead-claudia commented 3 days ago

@shaylew Okay, I took a step back, to focus more on the problem at hand. What about either of these possible solutions, using a .evaluateAndMarkReady() combined with scoping (#187)? Hooks would be called by watching and unwatching, and indirectly via computed .get() would get scheduled, and that method would execute and flush them.

Here's what I'm thinking at a high level (skipping around the value setting/propagating for brevity):

dead-claudia commented 1 day ago

Okay, I came up with a simpler alternative that punts the whole question to userland, while still preventing double-notification: https://github.com/tc39/proposal-signals/issues/222

For several promises, you can either aggregate them via Promise.all or use a dynamic batching mechanism (something I've wanted to see added to the language for years). The truly hard part isn't the immediate watching and signal collection management anyways, but the dependency tracking and knowing when to (and not to) watch.