Refs in useEffect in react-aria

[NaviTheCoderboi]

guys why do libs like react-aria have refs in useEffect deps even though they don't trigger side effects?

 useLayoutEffect(() => {
    // ...
  }, [scopeRef, restore, contain]);

scopeRef: RefObject<Element[] | null>

[snowystinger]

They sometimes make their way in, eslint can't catch them all, but they shouldn't cause any harm. Is it causing an issue for you?

[NaviTheCoderboi]

I was actually porting react-aria to svelte , so I was confused by refs in dependency array

[NaviTheCoderboi]

It should be removed as it can cause confusion

[babarbilal56]

The reason why React libraries like react-aria might include refs (like scopeRef) in the dependencies array of useEffect or useLayoutEffect—even though they don’t trigger side effects directly—has to do with how React manages re-renders and ensures that the effect is correctly triggered in the right scenarios.

Here’s why refs might be included:

React and Refs: While React refs don't cause re-renders directly when updated, they still refer to mutable DOM elements. React doesn’t trigger re-renders when the current value of a ref changes, but it ensures that the effect hook runs whenever the ref itself changes. This can be important if the hook logic depends on the presence or state of the referenced DOM element.

Ensuring Effect Execution: Including scopeRef in the dependencies of useLayoutEffect guarantees that the effect will run if the ref object itself changes. This is especially necessary if the library logic needs to track when the referenced DOM element (the scopeRef in this case) is either updated or reassigned. Even though changing the ref doesn’t directly trigger a re-render, the effect should still react to changes in the reference.

Stabilizing Side Effects: In some cases, it’s important to make sure the effect is run again even if the reference itself hasn't changed but the DOM or the context in which it operates has. Including refs in the dependency array helps manage this behavior.

Preventing Stale Closures: If the effect hook doesn’t depend on the latest ref, it could cause a stale closure, where the effect references an outdated version of the DOM element. Including refs in the dependency array ensures that the latest ref is always captured and avoids potential issues with stale closures.

In your example:
js
Copy
useLayoutEffect(() => {
// effect logic that depends on scopeRef
}, [scopeRef, restore, contain]);
scopeRef: The ref object itself could be changing, so it’s added to the deps array to ensure that the effect reacts to any changes in the referenced DOM element or when the ref is reassigned.
restore and contain: These might be state or props that directly trigger side effects in the effect, so they need to be included as well.
Even though refs don’t directly cause re-renders, they can still influence when and how side effects (like DOM manipulations or focus management) should be executed

[snowystinger]

Thanks for the comment. I disagree with this. If removing a ref from one of our dependency arrays causes the component to break, then that is a symptom of a larger logic problem. The ones in our effects are typically there by accident, possibly from before linting was updated to check for that, or snuck in during a large code review or any other of a number of reasons. It's not a big priority to remove them because they should never change and cause the effect to run an extra time. @NaviTheCoderboi has the right of it, it can muddle the readability.

The other reason that I disagree with this comment is that you cannot read from ref.current during render. That is unsafe in concurrent mode. I do not know if any of these were found in dependency arrays, but these would be ones to revisit, as they could be actual bugs in our code.

I checked every usage of scopeRef in FocusScope, I do not see any that would be a problem to remove. None of them should be changing, there is no reliance on the dom state. In some cases, however, a ref is passed into a function, and eslint is not smart enough to know that it's a stable ref. So, the ref is required in the dependency array.

react-spectrum/packages/@react-aria/focus/src/FocusScope.tsx

Line 511 in 988096c

}, [scopeRef]);

We could remove it and add the eslint-ignore, but I think that's worse since it would no longer warn about any legitimate cases that may come along.