fix: replace useEffect with top level ref setting in useStableCallback

[pavel-krasnov]

Motivation

Using useEffect to update the ref value causes potential issues with calling the callback before the first useEffect invocation. This happens for example in apps created with New Architecture. Setting the callback in the top level of the hook ensures that it is immediately ready to be called.

[gorhom]

we need the effect at least for the clean up

[pavel-krasnov]

What exactly do you need to clean the callback up for?

[gorhom]

  callbackRef.current = undefined;

freeing the ref object upon unmounting

[pavel-krasnov]

I understand. This is the first time I read anything about a need of cleaning up a ref object in a useEffect cleanup function. What advantages does it have?

[pavel-krasnov]

Ok, according to ChatGPT:

Without a cleanup function, callbackRef.current is never cleared. This might lead to unexpected issues if there are asynchronous tasks that outlive the component lifecycle, as callbackRef may retain a reference to the function even after the component has unmounted. This can cause memory leaks or even runtime errors if asynchronous code tries to call callbackRef.current after unmounting.

I will modify the pull request, thank you.

[fobos531]

@gorhom FYI, I can confirm that this diff fixes the issue of the sheet not appearing with dynamic sizing enabled on new architecture.

This might also fix #2015 & #2020

To recap, this is the version of the callback that works in my case.

import { useCallback, useEffect, useRef } from 'react';

// biome-ignore lint: to be addressed!
type Callback = (...args: any[]) => any;
/**
 * Provide a stable version of useCallback
 * https://gist.github.com/JakeCoxon/c7ebf6e6496f8468226fd36b596e1985
 */
export const useStableCallback = (callback: Callback) => {
  const callbackRef = useRef<Callback>();
  callbackRef.current = callback;

  const memoCallback = useCallback(
    // biome-ignore lint: to be addressed!
    (...args: any) => callbackRef.current && callbackRef.current(...args),
    []
  );
  useEffect(() => {
    return () => {
      callbackRef.current = undefined;
    };
  });
  return memoCallback;
};

[pavel-krasnov]

@gorhom FYI, I can confirm that this diff fixes the issue of the sheet not appearing with dynamic sizing enabled on new architecture.

This might also fix #2015 & #2020

To recap, this is the version of the callback that works in my case.

import { useCallback, useEffect, useRef } from 'react';

// biome-ignore lint: to be addressed!
type Callback = (...args: any[]) => any;
/**
 * Provide a stable version of useCallback
 * https://gist.github.com/JakeCoxon/c7ebf6e6496f8468226fd36b596e1985
 */
export const useStableCallback = (callback: Callback) => {
  const callbackRef = useRef<Callback>();
  callbackRef.current = callback;

  const memoCallback = useCallback(
    // biome-ignore lint: to be addressed!
    (...args: any) => callbackRef.current && callbackRef.current(...args),
    []
  );
  useEffect(() => {
    return () => {
      callbackRef.current = undefined;
    };
  });
  return memoCallback;
};

This is not totally correct in my opinion, because every time the callback changes, it might be called before the ref is updated.

Do we actually need this useStableCallback hook? I believe that the easiest way to fix those issues is to get rid of it in sake of simplicity.

[gorhom]

thank you @pavel-krasnov for submitting this PR <3

[ryskin]

It seems that this fixed the collapsing of the BottomSheet, but I might be wrong. Why did adding an inset and updating to version 5.06 solve the issue of the BottomSheet closing immediately after opening?

I don’t understand how this could be related. Why would the inset affect the BottomSheet closing? A value of 0 doesn’t work, but any non-zero value seems to fix the collapsing issue, but at 5.05 even inset not helps

[gaearon]

I don’t think this is the correct fix. Instead, try supplying the initial value to the ref in the useRef call. For future renders, applying it in an effect is more correct.