preactjs / signals

Manage state with style in every framework
https://preactjs.com/blog/introducing-signals/
MIT License
3.83k stars 95 forks source link

Sync between useSignal and its parameter #247

Open billybimbob opened 2 years ago

billybimbob commented 2 years ago

For both React and Preact, the useSignal hook implementation is currently defined as:

https://github.com/preactjs/signals/blob/ccb677bdb26ef836d3fac39d30d8f6371693e60f/packages/preact/src/index.ts#L334-L336

This implementation would cause the returned signal's .value to only be kept in sync with the value parameter on the very first call.

let param = 0;

// inside a compoenent

const num = useSignal(param); // num.value is 0

// re-render component with a modified param

param = 5;

const num = useSignal(param); // num.value is still 0

One way to get around this behavior is to immediately set the .value on the returned signal:

https://github.com/preactjs/signals/blob/ccb677bdb26ef836d3fac39d30d8f6371693e60f/packages/preact/src/index.ts#L70-L71

I am wondering, is there a specific reason that the .value is not set directly in the useSignal function? One possible limitation I can see is that batching multiple signal updates would become not possible.

I do think however that the benefit of synchronizing between a signal's .value and its value parameter outweighs that limitation.

KnisterPeter commented 2 years ago

The parameter to useSignal is probably meant like the parameter on useState. It's the initial value which is then tracked in the signal. The signal itself is valid as long as the component (which called useSignal) is mounted.

If you want to update the valid in the signal you should set it on the .value property, but usually not during rendering.

Do it either after data fetching (side-effect) or on user interaction (side-effect) and so on.

billybimbob commented 2 years ago

The parameter to useSignal is probably meant like the parameter on useState.

The similarities between useSignal and useState does make sense. However, I would argue that extending the funtionality of the useSignal parameter beyond just an initial value can be useful. One possible use case is keeping a signal in sync with a component parameter. I believe the pattern of keeping a signals's .value property in sync with the useSignal parameter is a common enough pattern that it would make sense to be baked into the useSignal hook itself.

If you want to update the valid in the signal you should set it on the .value property, but usually not during rendering.

I do agree that updating a signal's .value property would classify as effectful code, and should therfore be wrapped witin a useEffect call instead of directly being directly set during rendering. I did however, test the behavior difference between the two methods:

function EffectTest({ value = 0 }) {
  const source = useSignal(value);
  const timesTwo = useComputed(() => source.value * 2);
  const renders = useRef(0);

  useEffect(() => void (source.value = value), [value]);
  useEffect(() => void renders.current++);

  return (
    <div>
      <p>{source.value}</p>
      <p>{timesTwo.value}</p>
      <p>{renders.current}</p>
    </div>
  );
}

function SetTest({ value = 0 }) {
  const source = useSignal(value);
  const timesTwo = useComputed(() => source.value * 2);
  const renders = useRef(0);

  source.value = value;
  useEffect(() => void renders.current++);

  return (
    <div>
      <p>{source.value}</p>
      <p>{timesTwo.value}</p>
      <p>{renders.current}</p>
    </div>
  );
}

From what I've found, both methods keep the signal .value up to date. Also, setting the property directly during rendering is actually more performant. Based on how I understand Preact/React hook execution, the reason that useEffect is worse is because the useEffect code executes after rendering. Below is a (rough) description of how I understand both methods execute.

useEffect signal update:

During render signal update:

All of these details to properly utilize useSignal rely on knowing specifics on how signals operate, which is why I believe that the .value sync handling should be done directly in the useSignal hook.

eddyw commented 2 years ago

You can also pass a signal as prop rather than a value. It's similar how with useState you'd pass the value and setState function to a child component.

The difference with signals is that a component is also an effect (signal effect) so accessing a signal in a child doesn't necessarily mean that both the parent and child component will update when the signal changes.

function Test() {
   const s = useSignal(42);

   s.value = 123; // <<< This doesn't cause 'Test' to re-render

   return (
       <Foo s={s} />
   )
}
function Foo({ s }) {
   s.value; // <<< Accessing value will subscribe Foo to signal s

   return ...
}

Also yes, your SetTest is faster because initially source has no targets/subscribers so source.value assignment doesn't do anything besides setting the value. The component subscribes to changes on source afterwards when you do <p>{source.value}</p> which holds the signal's last value so it doesn't need to re-render.

billybimbob commented 2 years ago

You can also pass a signal as prop rather than a value. It's similar how with useState you'd pass the value and setState function to a child component.

Thanks for the tip. Passing down signal props definitely seems like the most straightforward approach, and at least for Preact, using signal props can also take advantage of the rendering and attribute optimization features. The signal hooks seem like a better alternative to the dependency array hooks.

Signal Hook Zombieness

I find that when using the signal hooks, specifically useComputed, they kind of have a "zombie-like" nature to them. What I mean by this is that replacing one useState or useMemo call with either useSignal or useComputed leads to further and further state replacements. This nudge towards further replacements is based on the fact that the .value of useComputed only updates when one of its signal subscriptions is updated, meaning that useComputed can only derive data from other signals.

This limitation means that trying to derive from a combination of a signal and a non-signal value can potentially not update correctly:

function Foo({ s, n }: { s: ReadonlySignal<number>, n: number }) {

    const combined = useComputed(() => s.value * n * n);
    // combined only recomputes if s is modified

    return <p>{combined}</p>;
}

There are a few options to fix the data staleness:

  1. Swap useComputed with useMemo
  2. Ensure that n is not modified
  3. Set n as the component key, so that when n changes, a new component is rendered
  4. Convert the n into a signal

Trying to use a mix of both signals and non-signals conflict with each other, Using 3rd party libraries become awkward to use, as whether or not the 3rd party library stores state using signals affects whether or not useComputed will update properly.

The conflict between signals and non-signals is a common enough problem that I believe a solution should be added to decrease the friction, either:

  1. Modify the behavior of useComputed to update on every render
  2. Add a compatibility layer to more easily convert a non-signal into a signal

Between the two options, the first option seems worse as the core computed behavior would be disrupted just to account for specific Preact/React behavior. The second option is why I initially brought up the point of modifying the useSignal hook so that the signal's .value property would be kept in sync with the value parameter.

I now realize that the semantics of the value parameter acting like useState is important, so I think it would make sense that an options argument is added to useSignal, or that an entirely new hook is added:

// modified useSignal

export function useSignal<T>(value: T, options?: { update: boolean }) {
    const source = useMemo(() => signal(value), []);

    if (options && options.update) {
      source.value = value;
    }

    return source;
}

// OR new hook added

export function useWatcher<T>(value: T): ReadonlySignal<T> {
    const source = useSignal(value);
    source.value = value;
    return source;
}
eddyw commented 2 years ago

Hm, useSignal is kinda like useState and useComputed is more like useMemo where the deps are signals. I think maybe it'd make sense for useComputed to also accept a deps argument:

const c = useComputed(() => {
  return s.value /* signal dep */ + dep1 /* React/Preact dep */
}, [dep1]);

So it'd re-compute if needed if the computed sources have changed but it'd also re-compute on re-render if the React/Preact deps also changed.

I think this makes sense because within a component, you may have 2 kinds of sources (dependencies), one may be other signals but also React/Preact dependencies such as props or other state.

billybimbob commented 2 years ago

I think maybe it'd make sense for useComputed to also accept a deps argument

That could be an option as well. Tbh, one part I like about useComputed is that the dependencies are inferred without having to manually specify them with an array.

This is definitely more of a matter of preference, but I feel that having a hook that essentially casts a non-signal into a signal, like for example useWatcher, fits more of the style that signals operate under:

// some hook to create a signal that updates when the value param updates

export declare function useWatcher<T>(value: T): ReadonlySignal<T>;

I view that a hook like useWatcher would be similar to how React's upcoming useEvent hook would operate, which essentially provides an escape from using dependency arrays:

const d = useWatcher(dep1);

const c = useComputed(() => s.value + d.value); // both s and d are signals, so c will update correctly
marvinhagemeister commented 2 years ago

You can turn @eddyw 's suggestion into a reusable hook:

export function useWatcher<T>(value: T): ReadonlySignal<T> {
  const signal = useSignal(value);
  signal.value = value;
  return signal;
}
dcporter commented 2 years ago

(Don’t update the value outside of a useEffect though, at least in React, as it may run the render but never commit, e.g. if it hits an error boundary in a nearby node.)

jamesarosen commented 1 year ago

@billybimbob said

The similarities between useSignal and useState does make sense

Indeed. I think useSignal(someProp) should really behave like the useWatcher(someProp) proposed here. What is the use-case for creating a Signal out of a (non-Signal) component prop or hook result but not wanting the Signal's value to update with the dependency?