useRef instead of useMemo

[bhovhannes]

This way it is shorter.
Also this implementation does not rely on useMemo which according to documentation may decide to skip caching under certain circumstances in the future.

[quisido]

I am happy that this passed the unit test that checks for the problem that useMemo was added to solve, but I am uncomfortable merging it until I fully understand the depths of the change.

Can you provide a test that would fail with the current useMemo implementation but will pass with the useRef implementation? Can you link to the part of the documentation that says that useMemo is unreliable in this use case? Are there benefits to useRef besides shorter code?

[bhovhannes]

I can't provide test speaking for useRef. Perhaps someone from React team can 😃

Here is quote from docs, taken from https://reactjs.org/docs/hooks-reference.html#usememo:

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.

[quisido]

That quote doesn't seem to be against the pattern used here. It's unfortunate that it's not a semantic guarantee, but performance takes a higher priority, as the purpose of passing a static reference to forceUpdate is to improve pure component performance. If React, in its wisdom, decides that rerendering pure components is a worthwhile trade for freed memory, this hook should not hinder that process.

The current implementation does work without useMemo and adds useMemo to optimize performance. It adds it for the semantics too, but it looks like the React team decided performance is of higher importance than semantics.

I'll leave this open to discussion, but I'm not currently convinced this change is in the best interest of Components relying on this feature.

[bhovhannes]

In my understanding useMemo was used here to maintain the same forceUpdate reference during component lifecycle. This is exactly the use case useRef is designed for - to maintain a single value during component lifecycle, that's why I changed that. If in the future React will decide to clear useMemo cache based on some conditions, current implementation may break something as forceUpdate reference will change occasionally.

…

On Thu, Apr 4, 2019, 10:57 PM Charles Stover ***@***.***> wrote: That quote doesn't seem to be against the pattern used here. It's unfortunate that it's not a semantic guarantee, but performance takes a higher priority, as the purpose of passing a static reference to forceUpdate is to improve pure component performance. If React, in its wisdom, decides that rerendering pure components is a worthwhile trade for freed memory, this hook should not hinder that process. The current implementation does work without useMemo and adds useMemo to optimize performance. It adds it for the semantics too, but it looks like the React team decided performance is of higher importance than semantics. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#6 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHGvCmxgP6-WpUqlxOc-HoF8puaZuOVnks5vdksYgaJpZM4ccC7g> .

[h3rmanj]

I agree with switching from useReducer to useState, since a reducer in this use case is overkill.

As for the forceUpdate function, I would suggest a third option, useCallback. While all three hooks results in a stable function identity, useCallback is the one that is actually designed for this scenario.

From the docs:

Pass an inline callback and an array of dependencies. useCallback will return a memoized version of the callback that only changes if one of the dependencies has changed. This is useful when passing callbacks to optimized child components that rely on reference equality to prevent unnecessary renders (e.g. shouldComponentUpdate).

And it would give the same result as useMemo:

useCallback(fn, deps) is equivalent to useMemo(() => fn, deps).

Except that according to the docs, useCallback doesn't say that it might decide to skip caching in the future.

useMemo is for memoizing expensive calls, which I wouldn't say wrapping a setState function once is.

useRef is designed for mutable variables, which again the setState wrapper is not supposed to be.

[bhovhannes]

@h3rmanj

useCallback(fn, deps) is equivalent to useMemo(() => fn, deps).

This still sounds suspicious to me because if these 2 are equivalent, if useMemo can skip caches, useCallback also can.

useRef is designed for mutable variables, which again the setState wrapper is not supposed to be.

Don't very agree with this statement. useRef returns a mutable ref object which is kind of box where one can store anything and stored value will be preserved during whole component lifetime.

Even without my something different understanding of what useRef is for, I still prefer to use useRef to be 100% sure that returned forceUpdate instance never changes because useCallback caching (or skipping the cache) is not well documented.

[h3rmanj]

While that being true currently, after some digging it seems that useMemo and useCallback have different plans for the future.

reactjs/rfcs#68 (comment)

useCallback is just a simple wrapper around useMemo, but we have ideas for how we could optimize that further in the future, either statically or using runtime techniques.

And they also don't want to invalidate useCallback for the wrong reasons facebook/react#14099 (comment), while the reason for useMemo skipping the cache is because it is meant for expensive calculations (and therefore also higher memory results), that does not apply for inline callbacks.

I worded the statement about useRef badly, what I meant was that while its use-cases are for single instance mutable variables, using it for just the single instance reason makes it just as overkill as using half of useReducers functionality. So my argument against it is just for readability and intent's sake.

But I might be wrong, my main point is just that going away from useMemo is the right choice, but I personally would prefer useCallback for readability and intent. Either way I support this PR.

[quisido]

I asked around, and I'm leaning towards useCallback being the correct implementation.

Something along the lines of:

useCallback(
  () => { dispatch(null); },
  [ dispatch ]
);

[quisido]

Given that this is now in master with useCallback, I'm closing this as duplicate.

Thank you so much for the discussion and address this issue.