Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Compiler Bug]: Mutating a ref returned from a function warns #29196

Closed
1 of 4 tasks
atomiks opened this issue May 21, 2024 · 5 comments
Closed
1 of 4 tasks

[Compiler Bug]: Mutating a ref returned from a function warns #29196

atomiks opened this issue May 21, 2024 · 5 comments
Assignees
Labels
Component: Optimizing Compiler Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Type: Bug

Comments

@atomiks
Copy link

atomiks commented May 21, 2024

What kind of issue is this?

  • React Compiler core (the JS output is incorrect, or your app works incorrectly after optimization)
  • babel-plugin-react-compiler (build issue installing or using the Babel plugin)
  • eslint-plugin-react-compiler (build issue installing or using the eslint plugin)
  • react-compiler-healthcheck (build issue installing or using the healthcheck script)

Link to repro

https://playground.react.dev/#N4Igzg9grgTgxgUxALhAMygOzgFwJYSYAEUYCAsgJ4BKCaAFAJRHAA6xRMCOsxpCtBowDc7AL7t2GbPkJEAEggA2SiExbsiROITA5OdIgF4SZKoKaiO-AKJo0CXPXVGAfBo5auaAHRxYXJj6JgDkOAh6IVZaYiLiIGJAA

Repro steps

When consuming mutable refs from custom hooks (or via props), the consuming components should be allowed to mutate them inside effects/event handlers.

function useMyRef() {
  return useRef('test');
}

function Hello() {
  const ref = useMyRef();
  useEffect(() => {
    ref.current = 'next';
//  ~~~ InvalidReact: Mutating a value returned from a function whose return value should not be mutated. Found mutation of `ref` (8:8)
  });
}

How often does this bug happen?

Every time

What version of React are you using?

19

@atomiks atomiks added Component: Optimizing Compiler Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Type: Bug labels May 21, 2024
@josephsavona
Copy link
Contributor

josephsavona commented May 30, 2024

Thanks for posting! The cause is very similar to #29160 (comment) — the compiler currently assumes that only values that come directly from useRef() are refs. As with #29160 , we will likely need to explore a heuristic for understanding which values may be refs, based on the ecosystem convention of "ref" or "-Ref".

@gsathya
Copy link
Contributor

gsathya commented Jun 3, 2024

we will likely need to explore a heuristic for understanding which values may be refs, based on the ecosystem convention of "ref" or "-Ref".

I'm looking into adding this. Assigning this to myself

gsathya added a commit to gsathya/react that referenced this issue Jun 17, 2024
If a component uses the `useRef` hook directly then we type it's return
value as a ref. But if it's wrapped in a custom hook then we lose out
on this type information as the compiler doesn't look at the hook
definition. This has resulted in some false positives in our analysis
like the ones reported in facebook#29160 and facebook#29196.

This PR will treat objects named as `ref` or if their names end with the
substring `Ref`, and contain a property named `current`, as React refs.

```
const ref = useMyRef();
const myRef = useMyRef2();
useEffect(() => {
  ref.current = ...;
  myRef.current = ...;
})
```
In the above example, `ref` and `myRef` will be treated as React refs.
gsathya added a commit to gsathya/react that referenced this issue Jun 18, 2024
If a component uses the `useRef` hook directly then we type it's return
value as a ref. But if it's wrapped in a custom hook then we lose out
on this type information as the compiler doesn't look at the hook
definition. This has resulted in some false positives in our analysis
like the ones reported in facebook#29160 and facebook#29196.

This PR will treat objects named as `ref` or if their names end with the
substring `Ref`, and contain a property named `current`, as React refs.

```
const ref = useMyRef();
const myRef = useMyRef2();
useEffect(() => {
  ref.current = ...;
  myRef.current = ...;
})
```
In the above example, `ref` and `myRef` will be treated as React refs.
gsathya added a commit that referenced this issue Jun 18, 2024
If a component uses the `useRef` hook directly then we type it's return
value as a ref. But if it's wrapped in a custom hook then we lose out on
this type information as the compiler doesn't look at the hook
definition. This has resulted in some false positives in our analysis
like the ones reported in #29160 and #29196.

This PR will treat objects named as `ref` or if their names end with the
substring `Ref`, and contain a property named `current`, as React refs.

```
const ref = useMyRef();
const myRef = useMyRef2();
useEffect(() => {
  ref.current = ...;
  myRef.current = ...;
})
```
In the above example, `ref` and `myRef` will be treated as React refs.
@maddymathan
Copy link

The error you're encountering happens because mutating a prop or a hook argument directly inside a hook is not allowed in React. The error arises because the target ref passed to the useResizeHandle hook is being directly mutated, which React warns against.

To resolve this, you should avoid directly modifying the target ref and instead use a local variable to track changes or update the target element in a way that doesn't involve directly mutating the passed reference.

Modified one:
`
import * as React from 'react';

export function useResizeHandle(
target: React.MutableRefObject<HTMLDivElement | null>,
) {
const bar = React.useRef(null);

React.useEffect(() => {
function resizeObject(event: MouseEvent | TouchEvent) {
if (bar.current && target.current) {
bar.current.style.width = "10px";
target.current.style.width = "10px"; // Make sure you are not mutating target itself but the element it refers to
}
}

document.addEventListener('mousemove', resizeObject, { passive: false });
return () => {
  document.removeEventListener('mousemove', resizeObject);
};

}, [target]); // target ref is a dependency here, but we don't mutate it directly

return {
dragging: true,
};
}`

Key Changes on the above code:
Avoid Direct Mutation instead of directly mutating target or target.current, the code now modifies the style properties of the element referred to by target.current, which is allowed.

Check for Existence and Added a check to ensure that bar.current and target.current are not null before accessing their properties.

Note:
1.Make sure that you are not using React version 19, as there is no such version at the time of writing. The latest stable version is React 18. You might want to confirm which version you are using.
2.Ensure that ESLint is configured correctly with appropriate rules to avoid these kinds of errors in the future.

@poteto
Copy link
Member

poteto commented Nov 13, 2024

This should be closed by #29916 - thanks @gsathya for fixing!

@poteto poteto closed this as completed Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Optimizing Compiler Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Type: Bug
Projects
None yet
Development

No branches or pull requests

6 participants