memoize useAsyncCallback ?

[hugomallet]

Hi,

useAsyncCallback returns a new object (and a new execute function) at each call even if the callback passed does not change. It causes re-renders.

[slorber]

Hi,

Agree I should memoize the execute function (and would accept a PR for that)

But for the returned object, you'd rather not pass it down to your tree and keep that logic near the top, use destructuring and only pass down the tree what is really needed. What's your motivation for passing down the full useAsyncCallback returned object to child components?

[hugomallet]

Actually it is the execute function that i need to be memoized.
But why not memoizing the entire object directly (ie the result of useAsyncInternal) ? As the parameters does not change, there is no need to redo anything. So re-calling "useAsyncInternal" is not necessary.

[slorber]

memoization has a cost (running shallowEqual(deps1,deps2)), particularly if you know for sure the result is likely to change, because the async state will change.

So optimizing for the re-rendering case also means de-optimizing slightly other usecases, + I don't really want to encourage leaking this lib's abstraction details to child components as I think it's an antipattern. Not sure I want to complicate existing code a bit, only to help promote an antipattern.

I'm thinking moving the lib to a different API closer to useState where there is [state,api], so that I can memoize more aggressively the api object. But still think it's not such a great idea to pass it as-is to child components.

Will memoize execute() anyway, there's no reason to not do it, thanks for telling me.

[hugomallet]

A shallow equal is very fast. And much more compared to recreating the result object and executing the code of useAsyncInternal.
You are probably right about not promoting passing the result object to a child. But it can be used also in hooks dependencies and would cause a recall of the hook that depends on it.

[slorber]

I know about this problem of useEffect being re-executed and agree that it can be confusing if using deps=[returnedObject] lead to a different result compared to deps=[execute].

Unfortunately I'm not really fan of this design decision of React. I also maintain react-navigation-hooks and it's the 1st source of trouble for users currently (react-navigation/hooks#40)

To me it would have been simpler and more convenient in the first place to use the deps array to just trigger effect execution, recomputations, and not "store" the closure. I've head Vue does does it this way and the stale closure problem is less an issue.

I opened an issue to discuss these design decisions with the React team. That lead to interesting discussions but unfortunately I'm still not sure how to handle this problem, in all the libs that I build :/
facebook/react#16956

[hugomallet]

For now what is the workaround to avoid recall of dependent hooks ? Creating a callback that uses a "ref" to execute ?

Nonetheless i think that memoization would improve perf

[slorber]

You can try this

const useEventCallback = (cb) => {
  let ref = useRef(cb);
  useEffect(() => ref.current = cb);
  return useCallback((...args) => ref.current(...args),[ref]);
}

const MyComp = () => {
  const asyncCallbackResult = useAsyncCallback(asyncFn);
  const execute = useEventCallback(() => asyncCallbackResult.execute());
}

Unlike regular hooks, it will not "store" the closure and always update it on re-render so you are sure to capture fresh variables (asyncCallbackResult)

[slorber]

Will think about memoizing the object too, you are right that it's more likely to improve the perf than degrade it, but as I'm thinking about changing return values slightly, it may be time to schedule a breaking change

[hugomallet]

Thanks for the workaround and quick replies.

[slorber]

no problem :)

[fabiosantoscode]

Just got hit by this. Using the return of useAsync as a useEffect dependency caused an infinite loop in my project. I was expecting the return value to be stable and it was quite surprising to find out it wasn't :)

If you'd like I can send a PR where this return is memoized, as well as the callback. If not for this major then for the next :)

[slorber]

Hi, @fabiosantoscode

Yes please, this is something I want to do anyway. As it didn't affect my projects I just didn't invest the time to fix it

[slorber]

execute() is now memoized

I don't think the result should be.

In the future, I'd like to refactor return value as [state, api] where api remains stable and could be used in deps array as a whole safely