tc39 / proposal-signals

A proposal to add signals to JavaScript.
MIT License
3.29k stars 57 forks source link

Consider allowing untracked reads during notification phase. #121

Open justinfagnani opened 5 months ago

justinfagnani commented 5 months ago

Preact signals allow a read during a subscription, ie:

signal.subscribe((value) => console.log(value));

This seems impossible with watchers, as this code throws:

const watcher = new Signal.subtle.Watcher(() => {
  console.log(signal.get());
});
watcher.watch(signal);

which is understandable, but an untracked read also throws:

const watcher = new Signal.subtle.Watcher(() => {
  console.log(Signal.subtle.untrack(() => signal.get()));
});
watcher.watch(signal);

I think this should be allowed. Otherwise it's impossible to write intentionally synchronous reactions.

alxhub commented 5 months ago

Otherwise it's impossible to write intentionally synchronous reactions.

This was the design intention. Signals can't be read during the notification phase because there is no guarantee the graph is in a consistent state - some Computeds might not be marked dirty yet, for example. This inconsistent read is a "glitch", and it's a design goal of the signals API to be glitch-free.

Note that it's possible for a wrapper to create a type of signal that can drive synchronous reactions:

class SyncState<T> extends Signal.State<T> {
  override set(value: T): void {
    super.set(value);
    runAnySyncReactions();
  }
}

Where runAnySyncReactions would execute any queued effects that were notified during the super.set() operation.

justinfagnani commented 5 months ago

I understand the intention, but I think this is too restrictive. In the case I'm developing here I'm only dealing with one signal, so there's no risk of a "glitch" locally. The point of the utility is to opt-out of the async scheduling that most signal access goes through when you specifically need synchronous updates.

justinfagnani commented 5 months ago

Maybe there could be a Preact Signals like signal.subscribe() method in addition to watchers?

shaylew commented 5 months ago

This was the design intention. Signals can't be read during the notification phase because there is no guarantee the graph is in a consistent state - some Computeds might not be marked dirty yet, for example. This inconsistent read is a "glitch", and it's a design goal of the signals API to be glitch-free.

It's feasible to do two passes of notification (one pass marks everything and accumulates callbacks to run, another calls notify), which gives you (at least some notion of) glitch-freeness again even when you get inside a Watcher callback. But that doesn't mean it's a good idea:

justinfagnani commented 5 months ago

If you really want this, you can implement it in userland by wrapping set

Wrapping set() isn't feasible if you want to be interoperable with signals in general. What we want is to be able to accept signals created directly or in any library. How else could someone use a future version of MobX or https://github.com/NullVoxPopuli/signal-utils ?

Wrapping also doesn't work with computeds.

littledan commented 5 months ago

I agree that we should discourage synchronous reactions. I think they will run into problems when crossing framework boundaries anyway; they are tricky to make good use of (and that's putting it charitably). You can always have a "backup" asynchronous reaction even if you "try" to react synchronously by wrapping set.

sorvell commented 5 months ago

There's clearly a lot of background thinking on this. Is there any write up or discussion of the related design decisions? That would be really helpful to review to facilitate better understanding.

It's not obvious why it's important that a watcher is notified before (any/all) signals become dirty. Reading during a "notification" seems natural and the entire reason that you asked to be notified of a change. Setting as a result of a notification or inside a computed, on the other hand, seems abusive.

sorvell commented 5 months ago

@justinfagnani

Wrapping set() isn't feasible if you want to be interoperable with signals in general.

It seems like this need was anticipated here, but it doesn't look like setPostSignalSetFn is exposed publicly atm.

sorvell commented 5 months ago

Related to https://github.com/proposal-signals/proposal-signals/issues/136

alxhub commented 5 months ago

postSignalSetFn was copy/pasted with the @angular/core/primitives/signals code. This library was designed as a shared base layer between Angular and Wiz's signal APIs, so it served as a nice starting point for the polyfill. Neither Angular nor Wiz use it currently for production, and its main purpose is as a potential hook for dev tools.

justinfagnani commented 5 months ago

I hit another case where I could use untracked reads during notification: I'm trying to build a log of changes to certain signals, and need to capture the value synchronously in order to see every value.

robbiespeed commented 4 months ago

@justinfagnani would the proposed functionality in #185 solve your use cases?

justinfagnani commented 4 months ago

At first glance, I think it could but it seems a little cumbersome to me compared to extending or modifying Watcher semantics. requestSettledCallback() appears very global, so to watch specific signals you have to build up something of a Watcher implementation yourself. A Watcher that runs the callback after the dirty-marking phase seems semantically more like what I want.

robbiespeed commented 4 months ago

Yeah I mention Watchers being delayed by default as a possibility in a later comment. The downside there is added overhead for the cases that were going to schedule something asynchronously anyway, it doesn't make much sense to add them to a queue only to then be added to a different queue. Though I think it would be easy to make such a watcher by extending the current one with requestSettledCallback.

const { requestSettledCallback } = Signal.subtle;
class SettledWatcher extends Watcher {
  constructor (notify) {
    super(() => requestSettledCallback(notify));
  }
}