Skip to content
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

Sync between useSignal and its parameter #247

Open
billybimbob opened this issue Oct 20, 2022 · 9 comments · May be fixed by #266
Open

Sync between useSignal and its parameter #247

billybimbob opened this issue Oct 20, 2022 · 9 comments · May be fixed by #266
Labels
enhancement New feature or request

Comments

@billybimbob
Copy link
Contributor

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

export function useSignal<T>(value: T) {
return useMemo(() => signal<T>(value), []);
}

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:

const currentSignal = useSignal(data);
currentSignal.value = data;

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
Copy link

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
Copy link
Contributor Author

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:

  • Render
  • Signal subscriptions notified of change
  • Signal .value evaluated & new signal subscriptions set
  • VDOM diff and paint
  • Effects run (signal is updated)
  • Signal subscriptions notified of change
  • Another render, repeat first step

During render signal update:

  • Render (signal is set)
  • Signal subscriptions notified of change
  • Signal .value evaluated & new signal subscriptions set
  • VDOM diff and paint

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
Copy link
Contributor

eddyw commented Oct 21, 2022

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
Copy link
Contributor Author

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
Copy link
Contributor

eddyw commented Oct 24, 2022

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
Copy link
Contributor Author

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 marvinhagemeister added the enhancement New feature or request label Nov 2, 2022
@marvinhagemeister
Copy link
Member

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
Copy link
Contributor

dcporter commented Nov 3, 2022

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

@billybimbob billybimbob linked a pull request Nov 4, 2022 that will close this issue
@jamesarosen
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants