signals icon indicating copy to clipboard operation
signals copied to clipboard

Bug?: useComputed doesn't update when given a new signal

Open jaskp opened this issue 2 years ago • 19 comments

I'm wondering if this is intended behavior or not.

Steps to preroduce:

  • Open https://stackblitz.com/edit/vitejs-vite-mebrre?file=src%2Fapp.jsx
  • Click Increase A
  • Observe that the large number in grams updates to reflect A
  • Click Show B in grams
  • Observe that the large number in grams doesn't update to reflect B
  • Click Increase B
  • Observe that the large number in grams still doesn't update to reflect B
  • Click Increase A
  • Observe that the large number in grams finally updates to reflect B

So it seems that useComputed can only start tracking a new signal when the previous one changes. If this is intended, it should be very well documented that you need to pass the same instance of the signal to get proper behavior.

jaskp avatar May 24 '23 21:05 jaskp

Hey, this feels similar to https://github.com/preactjs/signals/issues/361#issuecomment-1552229236, we should def document that! Thank you for calling this out. Basically this

JoviDeCroock avatar May 25 '23 06:05 JoviDeCroock

@JoviDeCroock I suppose that if Preact's signals are to be widely-used, some conventions should emerge. Such as, if a library exposes a component that takes signals as props, whose responsibility is to wrap them in useLiveSignal()?

It would also probably be useful to add the currently buggy uses of signals to tests so it can be verified that the problem is solved.

jaskp avatar May 25 '23 12:05 jaskp

And it also gets me thinking, I haven't experimented as much with useSignalEffect() so let's put that aside for a while and assume signals work perfectly until you use useComputed(). Wouldn't it be more robust ifuseComputed() was changed so that it could react to changes of the signal itself? The signal reference can only change if the component renders, so on every render, the closure could be evaluated with some no-op flag to find out if the signals are the same as before and if not, it would create a new computed() or something like that.

(Just thinking out loud)

jaskp avatar May 25 '23 12:05 jaskp

@JoviDeCroock does this also work if we have a global state and we are using the useContext hook to get the signal?

noopurphalak avatar Dec 28 '23 14:12 noopurphalak

main feature of useComputed is fact that it is not reexecuted on every rerender. The real way to check it - on babel plugin level, generate code which compares signals in scope with current ones

XantreDev avatar Dec 28 '23 16:12 XantreDev

But I don't think it's a good idea. I think it's better to have convention that you should provide stable signal refference into child component. Or if you can't - change child component key

XantreDev avatar Dec 28 '23 16:12 XantreDev

Btw, here is my hook that solve the issue

Here is fixed example: https://stackblitz.com/edit/vitejs-vite-xtukpt?file=src%2Fapp.jsx scrnli_12_28_2023_5-15-47 PM.webm

XantreDev avatar Dec 28 '23 16:12 XantreDev

@XantreGodlike Will this hook work in react?

noopurphalak avatar Dec 28 '23 16:12 noopurphalak

@noopurphalak All library work both in react and preact. This hook too

XantreDev avatar Dec 28 '23 16:12 XantreDev

@XantreGodlike I am asking specifically for state derived from useContext

noopurphalak avatar Dec 28 '23 16:12 noopurphalak

If you will change reference of signal in context provider - it will rerender bottom components, but signal in useComputed, useSignalEffect closures will not update

XantreDev avatar Dec 28 '23 16:12 XantreDev

@XantreGodlike Would you recommend defining signals and effects in a new JS file and exporting the signals to be used by multiple files instead of passing it to the Context API?

noopurphalak avatar Dec 28 '23 16:12 noopurphalak

If you're using Context for dependency injection it will work just fine. If you will do like this.

const Ctx = createContext(null)
const Child = () => {
  const sig = useContext(Ctx)
  // will not update, will be 2
  return useComputed(() => sig.value)
}

const A = () => {
  const sig1 = useSignal(1)
  const sig2 = useSignal(2)
  const useFirst = useSignal(false)
  
  return (
    <Ctx.Provider value={useFirst.value ? sig1 : sig2}>
      <Child />
    </Ctx.Provider>
  )
}

XantreDev avatar Dec 28 '23 16:12 XantreDev

So how do I get the updated state from useContext for useComputed or useSignalEffect to work? Is there any way to subscribe to signal changes in those hooks?

noopurphalak avatar Dec 28 '23 17:12 noopurphalak

import { useLinkedSignal } from '@preact-signals/utils/hooks'

const Child = () => {
  const sig = useLinkedSignal(useContext(Ctx))
  
  return useComputed(() => sig.value)
}

XantreDev avatar Dec 28 '23 18:12 XantreDev

import { useLinkedSignal } from '@preact-signals/utils/hooks'

const Child = () => {
  const sig = useLinkedSignal(useContext(Ctx))
  // will not update, will be 2
  return useComputed(() => sig.value)
}

I still don't understand the significance of useLinkedSignal if we can't use useComputed or useSignalEffect with its derived state. Maybe I am wrong, could you please clarify this for me. I will tell you an example of situation I am stuck in a REACT project:

I have a global state defined in authState.js as follows:

import { signal, effect, computed } from "@preact/signals-react";

export function createAuthState() {
  const authState = signal({
    userUUID: ""
  });

  const isLoggedIn = computed(() => !!authState.value.userUUID);


  return { authState, isLoggedIn };
}

Now I am passing the above state in the Context API as shown below:

<AuthContext.Provider value={createAuthState()}>
  <ConfigProvider theme={theme}>
    <App />
  </ConfigProvider>
</AuthContext.Provider>

In my Login.jsx I am updating my state like this:

state = useContext(AuthContext)

const updateState = (uuid) => {
  state.authState.value = {
    userUUID: uuid
  }
}

I also have a useSignalEffect in the Login.jsx which will redirect the user somewhere else if the user is already logged in like this:

useSignalEffect(() => {
  if (state.isLoggedIn.value) navigate("/dashboard")
})

This useSignalEffect does not seem to work when the state is updated in Login.jsx.

Please suggest a solution to this if possible.

noopurphalak avatar Dec 29 '23 03:12 noopurphalak

Don't create state at every render

<AuthContext.Provider value={useMemo(() => createAuthState(), [])}>
  <ConfigProvider theme={theme}>
    <App />
  </ConfigProvider>
</AuthContext.Provider>

XantreDev avatar Dec 29 '23 08:12 XantreDev

I tried that, still doesn't work

noopurphalak avatar Dec 29 '23 09:12 noopurphalak

I'm facing the same problem as @noopurphalak.

I have a signal and a computed signal globally shared via context, both created using useSignal and useComputed, but updating signals does not trigger the computed one

"@preact/signals-react": "^2.0.0",

allandcarv avatar Jan 15 '24 11:01 allandcarv