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

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

Open
jaskp opened this issue May 24, 2023 · 19 comments
Open

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

jaskp opened this issue May 24, 2023 · 19 comments

Comments

@jaskp
Copy link

jaskp commented May 24, 2023

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.

@JoviDeCroock
Copy link
Member

JoviDeCroock commented May 25, 2023

Hey, this feels similar to #361 (comment), we should def document that! Thank you for calling this out. Basically this

@jaskp
Copy link
Author

jaskp commented May 25, 2023

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

jaskp commented May 25, 2023

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)

@noopurphalak
Copy link

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

@XantreDev
Copy link
Contributor

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

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

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

@noopurphalak
Copy link

noopurphalak commented Dec 28, 2023

@XantreGodlike Will this hook work in react?

@XantreDev
Copy link
Contributor

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

@noopurphalak
Copy link

@XantreGodlike I am asking specifically for state derived from useContext

@XantreDev
Copy link
Contributor

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

@noopurphalak
Copy link

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

@XantreDev
Copy link
Contributor

XantreDev commented Dec 28, 2023

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>
  )
}

@noopurphalak
Copy link

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?

@XantreDev
Copy link
Contributor

XantreDev commented Dec 28, 2023

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

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

@noopurphalak
Copy link

noopurphalak commented Dec 29, 2023

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.

@XantreDev
Copy link
Contributor

Don't create state at every render

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

@noopurphalak
Copy link

I tried that, still doesn't work

@allandcarv
Copy link

allandcarv commented Jan 15, 2024

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",

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

Successfully merging a pull request may close this issue.

5 participants