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

useSignal should not use useMemo #307

Open
reosablo opened this issue Feb 19, 2023 · 6 comments
Open

useSignal should not use useMemo #307

reosablo opened this issue Feb 19, 2023 · 6 comments

Comments

@reosablo
Copy link

The current implementation of useSignal uses useMemo.

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

React document says to write the code so that it still works without useMemo. Preact might be the same.

You may rely on useMemo as a performance optimization, not as a semantic guarantee. In the future, React may choose to “forget” some previously memoized values and recalculate them on next render, e.g. to free memory for offscreen components. Write your code so that it still works without useMemo — and then add it to optimize performance.

Without useMemo, useSignal will create a new signal instance every time when re-rendering occurs and lose the previous value.

Also, useComputed uses useMemo. There's no problem I think.

export function useComputed<T>(compute: () => T) {
const $compute = useRef(compute);
$compute.current = compute;
return useMemo(() => computed<T>(() => $compute.current()), Empty);
}

@reosablo
Copy link
Author

The implementation could be like the bellow.

export function useSignal<T>(value: T) { 
	const $signal = useRef<Signal<T>>();
	return $signal.current ??= signal<T>(value);
} 

@billybimbob
Copy link
Contributor

The implementation could be like the bellow.

export function useSignal<T>(value: T) { 
	const $signal = useRef<Signal<T>>();
	return $signal.current ??= signal<T>(value);
} 

At least for Preact hooks, utlizing the useRef hook vs the useMemo hook makes no difference, because useRef utilizes useMemo under the hood.

The useRef implementation may still be worthwhile for specifically React version of useSignal.

@XantreDev
Copy link
Contributor

In reality useMemo computes once.
btw you can use useState
useState(() => signal(item))[0]

@jamesarosen
Copy link

jamesarosen commented Mar 20, 2023

Related: the current useMemo implementation also makes it awkward to create a Signal with input that varies based on React props or the results of other React hooks.

The best option seems to be to create a Signal with the initial value and then immediately set it again using useSignalEffect:

function useMyHook(someValue: SomeType) {
  // value set during signal creation:
  const signal = useSignal<SomeType>(someValue)

  // update value if `someValue` changes, since `useMemo` hides that change:
  signal.value = someValue

  return computed(() => {
    return doSomethingWith(signal.value)
  })
}

Regardless of whether useSignal uses useMemo, it should understand that its input may vary over time and should update the signal's value accordingly. The naive useMemo implementation would cause the destruction and recreation of the Signal itself, which defeats the purpose:

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

A slightly more sophisticated implementation would bundle a useMemo and a useEffect or useSignalEffect together to achieve the right result.

Or, in other words, why wouldn't we want useSignal(someProp) to behave just like the proposed useWatcher?

Later

Is there perhaps a use-case for creating a Signal from a prop, then manually controlling when it updates? I can envision the code, but I'm not sure why I would want it:

function MyComponent({ foo }) {
  const fooSignal = useSignal(foo); // always "Foo1" unless we update it here

  useEffect(() => {
    if (someCondition(foo, bar)) {
      fooSignal.value = foo
    }
  }, [foo])

  return <div>foo: {foo}, fooSignal: {fooSignal.value}</div>
}

But why wouldn't you model that as a computed based on a fooSignal that follows foo?

Further edit

I retract this. A really simple use-case that requires a different useSignal and useWatcher is tracking the previous value so you can, e.g. unsubscribe to some stream based on the previous value and, simultaneously subscribe to a new stream based on the new value.

@skybrian
Copy link

I think there is no problem 1, but since a search led me here and I just figured this out, maybe my explanation will help others:

This is all about lifetimes. The reason we have hooks is that parts of a component live longer than a render of that component. We want to re-render and get a view that's logically the same. 2

All we want for signals is that they have the same lifetime as the component. This can be achieved in different ways, which are all equivalent:

  • a useState() hook saves an object until you change it. If you never change it, then that's what we want.

  • a useMemo() hook saves an object until a dependency changes. If it has no dependencies, that's what we want.

  • a useRef() hook saves an object until a rendered element changes it with with 'ref' attribute. If you never do that, that's what we want.

useState seems like the most idiomatic way to do this. A signal is a kind of state with more features and updated in a different way, so we don't want the second return value.

Footnotes

  1. If the implementation of useMemo ever changes in Preact, updating useSignal would be easy to do, so someone can do it then.

  2. This is similar to how reloading a web page should often show the same content, even though technically, the web page was recreated. (The URL is the same and the server-side persistent data is the same.)

@zetaraku
Copy link

In reality useMemo computes once. btw you can use useState useState(() => signal(item))[0]

I like this solution. It's logically correct and concise.

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

No branches or pull requests

6 participants