tc39 / proposal-signals

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

Allow `state.set` during `Watcher(notify)` #177

Open robbiespeed opened 1 month ago

robbiespeed commented 1 month ago

This is important for composing reactive selectors like:

declare function createSelector<T, U>(
  input: Signal<T>, mapper: (isSelected: boolean) => U
): (matchValue: T) => Signal<U>;

const classNameSelector = createSelector(selectedIdSignal, (isSelected) => isSelected ? "active" : "");

const itemClassName = classNameSelector(itemId);

Without it the derived itemClassName cannot be updated synchronously when selectedIdSignal changes. This can lead to triggering 2 successive renders vs 1.

Allowing this should be possible without risk of corrupting the signal graph, since it can only mark more nodes dirty.

dead-claudia commented 1 month ago

IMHO, state.set shouldn't be restricted at all in the watcher. Not if you block the watcher correctly.

Only time it even could make sense IMHO is when executing a Computed that depends on it. But even that has a use case: making the computed dirty after its .get() could be used by a caller tracking dirtiness to, say, loop back and re-invoke it after processing the return value. (This could be used to implement multiple renders, for example.)

sorvell commented 3 weeks ago

For reference, just noticed this is optional in the Angular implementation (from which this proposal's current polyfill seems to be copied).