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

[React 19] eslint-plugin-react-compiler warning on destructured props mutation #29093

Closed
Janpot opened this issue May 16, 2024 · 4 comments
Closed

Comments

@Janpot
Copy link

Janpot commented May 16, 2024

Summary

Trying out eslint-plugin-react-compiler, it warns on the following code:

function Foo({ ...props }) {
  props.foo = false;
  // Mutating component props or hook arguments is not allowed. Consider using a local variable instead
  return <div />;
}

Since props is destructured it's not mutating the original properties, so I believe this is a false positive.

@HoseinKhanBeigi
Copy link

The ESLint rule is designed to prevent mutation of props within a component, which can lead to unpredictable behavior and bugs. Even though destructuring props may create a new object, mutating the destructured props is still considered a bad practice in React.

function Foo({ foo, ...rest }) {
  // Create a new object with foo set to false, and spread the rest of the properties into it
  const modifiedProps = { foo: false, ...rest };
  
  // Spread the properties of modifiedProps as props to the div element
  return <div {...modifiedProps} />;
}

@Janpot
Copy link
Author

Janpot commented May 16, 2024

Even though destructuring props may create a new object, mutating the destructured props is still considered a bad practice in React.

Why is it bad practice? I assumed the problem was that a render function shouldn't mutate the props object because react wants to use it still after rendering, e.g. to allow rendering twice with the same properties.

@josephsavona
Copy link
Contributor

Thanks for filing this. You’re right that technically, destructuring props in this way will create a new object. However, this is only a shallow copy - so it’s okay to set a top-level property, but not for example to mutate the values of the properties.

Our current model for mutability doesn’t support shallow copies, given how error prone they are. So for now we don’t expect to change the implementation here. Basically, it’s technically safe but error prone. We’ll consider if we can specialize the error message in this case.

@Janpot
Copy link
Author

Janpot commented May 17, 2024

🤦 Oh ofcourse, it's about tracking mutations. Makes sense, thanks for explaining.

BlackRule pushed a commit to BlackRule/rs.school-React that referenced this issue Jul 4, 2024
BlackRule pushed a commit to BlackRule/rs.school-React that referenced this issue Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants