useForwardedRef incorrectly updates the ref during the render phase

[denis-sokolov]

In #5462 the function useForwardedRef was changed so that now the ref is mutated during the render phase of React. This violates React’s expected invariant that render functions have no side-effects and conflicts with Concurrent React.

In some cases React warns about that in the console with: “Cannot update a component while rendering a different component. To locate the bad setState() ...”

In the latest master, notice how one updateRef is correctly in an effect, and the other is outside of an effect:

grommet/src/js/utils/refs.js

Lines 15 to 21 in 7c45cc9

	export const useForwardedRef = (ref) => {
	const innerRef = useRef(null);
	updateRef(ref, innerRef);
	useLayoutEffect(() => updateRef(ref, innerRef));
	useEffect(() => updateRef(ref, innerRef));
	return innerRef;
	};

PR change.

Expected Behavior

useForwardedRef should only mutate ref in the useEffect.

Actual Behavior

useForwardedRef mutates ref outside of an effect.

[jcfilben]

Linking react docs section that advises not to do this: https://beta.reactjs.org/reference/react/useRef#caveats

[MikeKingdom]

Linking react docs section that advises not to do this: https://beta.reactjs.org/reference/react/useRef#caveats

Doesn't this statement allow this usage, ie. for initialization?

Normally, writing or reading ref.current during render is not allowed. However, it’s fine in this case because the result is always the same, and the condition only executes during initialization so it’s fully predictable.

[denis-sokolov]

@MikeKingdom, the code in useForwardedRef updates ref potentially at any render, not only the first one. If innerRef has been changed, the next time we render, useForwardedRef will mutate ref.

[MikeKingdom]

@MikeKingdom, the code in useForwardedRef updates ref potentially at any render, not only the first one. If innerRef has been changed, the next time we render, useForwardedRef will mutate ref.

I think you could avoid this and make it just like the example in the react docs reference if you changed useForwardedRef to do

if (ref?.current === null) updateRef(ref, innerRef);

but leave the use*Effects as is.
It would take some testing to see if this caused issues. I've run into some tricky issues with the timing of this as it is.

[denis-sokolov]

@MikeKingdom, I can confirm that does not fix it.

In the example in the docs the ref is always populated in the initial render and is never set to null again. Even though the docs technically allow setting it in the initial render, it is effectively invisible from the outside, it’s as if we set the initial value from the start.

In the code of useForwardedRef, even with the conditional, updateRef will still be called in the subsequent renders, because innerRef can be null for more than one render, or it can turn to a null during the component’s lifecycle.

I can confirm that removeing the entire call to updateRef that is outside the effect does remove the React warning.

[MikeKingdom]

@MikeKingdom, I can confirm that does not fix it.

I can confirm that removeing the entire call to updateRef that is outside the effect does remove the React warning.

Fair enough. It looks like a more acceptable way to use a forwarded ref is with useImperativeHandle().
For example, from https://www.carlrippon.com/using-a-forwarded-ref-internally/

React.useImperativeHandle(ref, () => internalRef.current);

Thoughts on if this avoids the problem with invariance and Concurrent React?

[denis-sokolov]

useImperativeHandle should be a good solution!

[taysea]

Closed by #6564