facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
228.8k stars 46.82k forks source link

[Compiler Bug]: eslint-plugin-react-compiler errors when updating initialization of ref.current #30782

Open jeremy-code opened 2 months ago

jeremy-code commented 2 months ago

What kind of issue is this?

Link to repro

https://playground.react.dev/#N4Igzg9grgTgxgUxALhAHRFMCAEcA2AlggHYAuGA3GiTYQLYAOEMZOwOWCASggGY4Avjj4wI9HBhgIAhnAohqtEnAgkwbAKoA5AJJ6AKroCCAGV0AtAKIARHAF4cAZQCe9AEYR8ACgw79ukZmlrYYAJRKNAgAHsyseGoanNgA8iq4jgA8BgA0AHzehCSEZIQy+ABq5VAIyDgGOAA+ON7eYQ559WHt9p3ANDgJ6mzSAo5cvAKZ9U04ZC6MCBAC-oYm5tZ2nd6rgeshNhE0AziEAt6jAHRwsNLkDvaOu0Ebtu39JIODVzcwd2yOeaLZanYqlcpVfA1B6ODB8KAqUpqDA4AD8oJKZUq1QQbRwdSKmIhOKUg0Ex0+OGkZFgnx+t1IZCUgkiJBicTYABN+DIoPg2PDEYQ1DgALIuYyMRh4j7fBA0mCfTKcwgANzyAAkEPh8BAcAB1Fj4TmZAD0KvVzJoIEEQA

Repro steps

  1. Initialize useRef with some dummy value (e.g. null, Symbol, undefined, etc.) to be changed after initialization/during render.

  2. Update ref.current by checking whether it is equivalent to its initial condition (as per documentation: useRef#Avoiding recreating the ref contents)

    • To solve it, you may initialize the ref like this instead:

      function Video() {
      const playerRef = useRef(null);
      if (playerRef.current === null) {
      playerRef.current = new VideoPlayer();
      }
      // ...
    • 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.

  3. eslint-plugin-react-compiler gives error

InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef)

How often does this bug happen?

Every time

What version of React are you using?

19.0.0-rc-1d989965-20240821

jeremy-code commented 2 months ago

Related: #30716

parthnegi21 commented 2 months ago

Try this

import { useRef, useState, useEffect } from "react";

const UNINITIALIZED = Symbol("UNINITIALIZED");

export const useOnce = <T,>(initialValue: T | (() => T)) => { const ref = useRef < T | typeof UNINITIALIZED > (UNINITIALIZED); const [value, setValue] = useState < T | typeof UNINITIALIZED > (UNINITIALIZED);

useEffect(() => { if (ref.current === UNINITIALIZED) { const resolvedValue = typeof initialValue === "function" ? initialValue() : initialValue; ref.current = resolvedValue; setValue(resolvedValue); } else { setValue(ref.current); } }, [initialValue]);

return value; };

export default function MyApp() { return

Hello World
; }

State Initialization: Added a state value to store the initialized value.

useEffect: The ref is now initialized inside the useEffect hook, which sets the value both in the ref and in the state.

Return Value: The hook returns the state value, ensuring that the initial render doesn't access ref.current

josephsavona commented 1 month ago

Thanks for posting. This is a known limitation of the new linter.

hlege commented 1 month ago

i also run into the problem in a different scenario: when passing the ref object to a custom hook the compiler is not optimizing my code. if i remove the ref access it works as expected.

// pass the ref object to a custom hook and it is preventing the optimizations.  
const mergedRefs = useMergedRef<HTMLDivElement>(ref, setPopperElement);
iahu commented 1 month ago

Do you deprecated to use useRef? if not, how to use it?

const panelRef = useRef<HTMLDivElement>(null);
const width = panelRef.current?.clientWidth ?? 256; // eslint error here
alisherks commented 1 month ago

Is there any available workaround for this? This limitation also affects the useImperativeHandle.

alisherks commented 1 month ago

To anyone encountering optimization issues with the usage of useImperativeHandle, you can place the forwardedRef inside a useState hook.

const [refs] = useState(() => {
    return { forwardedRef };
});

This works against latest experimental release (0.0.0-experimental-4e0eccf-20240830). It probably breaks the rules, but so far didn't encounter serious issues.

nkalpakis21 commented 1 month ago

built an app to get paid for this PR https://www.n0va-io.com/discover/facebook/react

ravicious commented 1 week ago

I think this is addressed in 19.0.0-beta-8a03594-20241020. I have a hook that looks like this:

export function useLogger(name: string) {
  const loggerRef = useRef<Logger>(null);
  if (loggerRef.current === null) {
    loggerRef.current = new Logger(name);
  }

  return loggerRef.current;
}

The linter rule complains only about that .current in the return statement. However, the conditional needs to have exactly this form of fooRef.current === null, otherwise the rule will report a false positive.