[core] Switch back to useCallback to avoid ESLint error

[albarv340]

Revert back to previous implementation from #39078, with some minor adjustments. The reason for this is that eslint-plugin-react-compiler gives an ESLint error when ref.current is being accessed inside the useRef which from our understanding can cause inconsistencies. If this inconsistency is already handled in other ways, then the leaner version from #39078 can be kept, and an explicit ignore of this error could be added. This warning was noticed when working on #44591, and we felt it might be relevant to get rid of this ESLint warning as well.

• I have followed (at least) the PR section of the contributing guide.

[mui-bot]

Netlify deploy preview

https://deploy-preview-44592--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against ac44245

[romgrk]

which from our understanding can cause inconsistencies

Can you clarify this? I don't understand.

[albarv340]

Can you clarify this? I don't understand.

We based this pull request mostly off of the ESLint warning given by eslint-plugin-react-compiler. From our understanding, it tries to catch the pitfall mentioned on this page: https://react.dev/reference/react/useRef. So the inconsistencies we were alluding to were the breaking of React's expectation that the body of a component behaves like a pure function. But we are not entirely sure this expectation is broken, we base it entirely on the ESLint warning, but that might be a false positive. Perhaps you know more on this topic?

[romgrk]

React's expectation that the body of a component behaves like a pure function

Kinda misleading terminology. Hooks are fundamentally impure functions, because they depend on hidden global state that isn't passed through parameters. So I'm guessing they're meaning "pure with the exception of React hooks".

But either way, I still don't understand how accessing ref.current is a potential issue. If you want to make the change, could you clarify what the potential issue is? I'd rather not blindly trust the eslint plugins, they're full of false positives. It could be worth it to open an issue upstream to ask them to clarify the eslint rule and/or the docs.

[albarv340]

We assume the issue would be the one mentioned on the documentation we sent. We admit we do not fully understand it, but earlier in the code when ref.current is accessed, it is wrapped in an effect hook, while the latter one is not:

useEnhancedEffect(() => {
  ref.current = fn;
});

So wouldn't the usage of current in the latter context also need to be inside a similar hook?

return React.useRef((...args: Args) =>
  // @ts-expect-error hide `this`
  (0, ref.current!)(...args),
).current; // < -- This current is not inside of a hook

If there is a good reason for this, we understand this pull request might not be necessary, but it looks like something is potentially strange with the current implementation.

[romgrk]

// < -- This current is not inside of a hook

Aren't you mixing element refs with general-purpose refs? Because for an e.g. Ref<HTMLDivElement> used as <div ref={ref}>, it makes sense that ref.current access needs to be restricted to within hooks because the HTML element is only attached after the component is mounted (and before hooks run), but for a general-purpose ref that we are initializing, those rules don't make sense. We are initalizing const ref = useRef(fn), therefore ref.current is never invalid.

[albarv340]

Aren't you mixing element refs with general-purpose refs?

Yes, it is entirely possible that both eslint-plugin-react-compiler and we are confusing the two refs. So the switch to a useCallback might not be entirely necessary.

With your explanation, it makes us question whether the hook that updates the ref.current is necessary if it is already initialized to fn?

const ref = React.useRef(fn);
useEnhancedEffect(() => {
  ref.current = fn;
});

Other than that, we are also curious about the comma operator (0, ref.current!) and if it plays any real role in the code? This also goes for the non-null assertion operator after ref.current, since as you explained it is never invalid.

[romgrk]

Yes, those things are necessary for the hook to update & behave properly, see the react docs for useEffect and JS docs for this binding rules to get more information. I would close this PR unless we can articulate a valid reason for the change.

Note that if the compiler is choking on this pattern and produces otherwise worst code, that could be a valid reason to make the change anyway, but I'd validate that assertion first and check upstream for a fix if that's the case.