preactjs / signals

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

Incompatibility with React useEffect #491

Closed JonAbrams closed 6 months ago

JonAbrams commented 6 months ago

I'm using @preact/signals-react along with the babel transformer and am running into an issue with React's useEffect. The official react lint rule react-hooks/exhaustive-deps doesn't play nicely with signals.

Consider this example:

import { signal } from '@preact/signals-react';
const countSignal = signal(0);

export function Counter({ multiplier: number }) {
  useEffect(() => {
    localStorage.setItem('countWithMultiplier', `${multiplier * countSignal.value}`);
  }, [multiple, countSignal.value]);

  return (<div>
    {multiplier * countSignal.value}
    <button onClick={() => countSignal.value++}>Add One</button>
  </div>);
}

The above will fail the react-hooks/exhaustive-deps check because countSignal.value in the list of useEffect dependencies is declared an error:

Outer scope values like 'countSignal.value' aren't valid dependencies because mutating them doesn't re-render the component

I know that there exists a useSignalEffect hook to allow a component to run side effects on a signal's change, but it's only for signals, it can't trigger on prop changes too (like in the example).

This can be worked around by assigning the value to a local variable first (const countValue = countSignal.value), but that's annoying. Plus before you even remember to do that, an eslint autofix would just silently remove the countSignal.value dependency from useEffect, leading to a potential bug.

One potential fix could be to add an optional dependency array to useSignalEffect so that it can also trigger on non-signal changes. Also, useSignalEffect isn't documented, so there's that too.

Update: Fixed example, now using a signal created out of scope of the component to reflect the actual case were the linter raises a warning.

XantreDev commented 6 months ago
  1. It's not incompatibility.
  2. eslint plugin says truth and this issue is a regular one, when you're working with any kind of objects. The real way to handle it look like this:
    
    import { useSignal, useComputed, useSignalEffect } from "@preact/signals-react";
    import { useLinkedSignal } from "@preact-signals/utils/hooks";

export function Counter({ multiplier }: { multiplier: number }) { const countSignal = useSignal(0); const multiplierSig = useLinkedSignal(multiplier); const total = useComputed(() => multiplierSig.value * countSignal.value);

useSignalEffect(() => { localStorage.setItem("countWithMultiplier", ${total.value}); });

return (

{total}

); }



https://tsdocs.dev/docs/@preact-signals/utils/0.14.1#md:uselinkedsignal

Here is implementation of linked signal: https://github.com/XantreGodlike/preact-signals/blob/main/packages/utils/src/hooks/useLinkedSignal.ts
rschristian commented 6 months ago

The official react lint rule react-hooks/exhaustive-deps doesn't play nicely with signals.

Expecting official lint rules to work with an unofficial state primitive seems a tad bit overly hopeful to me.

If you want to restrict yourself to lint rules that aren't fully aware of the context of your code, then you may need to use one of your workarounds. I don't think adding a dep array to useSignalEffect is a great option here.

As mentioned, you could just make your non-signal prop into a signal. I know you have a simplified example there, so that might not be viable all the time, but is an option.

Also, useSignalEffect isn't documented, so there's that too.

It sorta is but certainly not well. I think our docs here need some time investment for sure (and the Preact site needs a note saying it's mainly centered around Preact usage, but I digress).

JonAbrams commented 6 months ago

Just spitballing here, but how about a separate eslint plugin/rule based on react-hooks/exhaustive-deps but detects/allows signal values? It's been about a decade since I wrote an eslint plugin, so I'm a little rusty.

rschristian commented 6 months ago

Sounds like a reasonable solution, but I don't think we have any interest or capacity to build/maintain that, if that's what you're asking.

JonAbrams commented 6 months ago

That's understandable, I think I'll take a crack at it when I have some free time.