eslint-plugin-react: Add "destructureInSignature: never" to destructuring-assignment

Nope, that’s the thing. My extensive experience tells me that this is indeed an actual bug, and at times a catastrophic one, since mutating props invalidates any predictions about React rendering.

I completely understand your concern. Mutating the props object can indeed lead to unwanted behavior and introduce bugs in an application.

However, I’d like to mention that the no-param-reassign ESLint rule was specifically created to prevent parameter mutation. You can leverage the props option of this rule to ensure that any attempt to mutate a parameter will be caught and prevented, like this:

function Component(props) {
    props.foo = "value";  // Error
}

Additionally, when you’re destructuring your params inside the signature, it can be a bit challenging to assign default values. In such cases, it’s much easier to do so when performing the destructuring outside, like this:

function Component(props) {
    const { foo = 'bar' } = props;
}

Another advantage of destructuring outside the signature is that you cannot mark the props ‘as const’ when doing it inside, which helps prevent accidental reassignments:

function Component({ foo }) {
    foo = 'acme'  // Works, which might lead to bugs
}

function Component(props) {
    const { foo } = props;
    
    foo = 'acme'  // Error, making it safer
}

In the end, both approaches have their pros and cons, and it’s essential to choose the one that best suits your specific use case. This is why I believe it’s valuable to support and allow both approaches within the ESLint rules, as they cater to different coding styles and scenarios.

When it’s purely style, yes! However, never destructuring in the signature actually makes your code worse and less robust, which will cause bugs in your code - so it’s not purely a matter of style, and it would be reckless for me to enable that.

If WebStorm doesn’t work well with an idiomatic style, WebStorm is broken, not the style - file a bug with them.

Obviously if you want to access the props object then it’s fine to move it to the body, but passing props objects to children wholesale is an antipattern.

Nope, that’s the thing. My extensive experience tells me that this is indeed an actual bug, and at times a catastrophic one, since mutating props invalidates any predictions about React rendering.

Hey @ljharb! 👋🏻

I’ve thoroughly read the entire thread once again to make sure I didn’t miss any points

From what I gathered, it seems there’s a concern that some people might accidentally mutate the props object without fully understanding its implications. While this is a valid concern, I believe it’s more of a potential misunderstanding rather than an actual bug.

Is there any other potential “bug” that you think I might have missed?

wever, never destructuring in the signature actually makes your code worse and less robust, which will cause bugs in your code - so it’s not purely a matter of style, and it would be reckless for me to enable that.

It’s so untrue…

@ljharb Just to be clear about the spread in the function signature won’t protect you from bugs.

What a spread operator does? It deep copies the data if it is not nested. For nested data, it deeply copies the topmost data and shallow copies of the nested data. Destructuring in the signature just protects me from reference the original props object but allows me to reference nested data of props object. This is only a partial protection in case there is a nested data in the props.

It is entirely the responsibility of the developer to protect props from mutations. You may still need to be careful when modifying nested objects or arrays to avoid unintentionally modifying the original object.

While it’s true that using the spread operator and destructuring can help protect your code from unintended mutations, not using destructuring in the function signature does not necessarily make your code worse or less robust.

In conclusion, while using the spread operator and destructuring in the function signature can provide some benefits, it’s up to the developer to choose their preferred coding style and ensure they are handling data carefully to prevent unintended mutations.

Destructuring in the signature has concrete benefits, whether using TS or not - it avoids the props object being accessible at all, which avoids a huge class of bugs caused by mutating or exposing that mutable props object.

This isn’t a mere aesthetic difference, and I don’t think it would be a good idea to have a rule that encouraged it.

@RomainLanz have you read the entire thread? not having it in the signature can cause bugs, so it’s not purely a style choice.

So what is the status of this issue? Why is it closed?

It seems fair to ask for a never option for the destructureInSignature. Why would we be able to force the destructuring in the signature but not force the destructuring to not be in the signature?

Please reconsider this issue as it seems to be a nice-to-have feature and should not break anything.

Sorry, guys. I really don’t like destructuring in signature too.

My reasons:

• WebStorm doesn’t work good with indentation when cutting and pasting props for reordering in construction like this;

  function Foo({
    a,
    b,
    c,
    d,
    e,
    …
    z,
  }) {
  

• destructuring in signature sucks for a few parameters occupying multiple lines while I prefer it to be more compact like this (depends on max line length):

  function Foo(props) {
    const { a, b, c, d, e } = props;
  }
  

• sometimes I want to access props. in IDE in order to watch which properties are also available for me as well as passing partial or the whole props to children components easily.

Destructuring in the signature has concrete benefits, whether using TS or not - it avoids the props object being accessible at all, which avoids a huge class of bugs caused by mutating or exposing that mutable props object.

Nobody uses props.x, because we’re using restructuring on the next line

This isn’t a mere aesthetic difference, and I don’t think it would be a good idea to have a rule that encouraged it.

There is

We have components with 10-20 props and putting them in a function definition is less readable for me.

Looks like basically, you’re saying “we don’t use it - nobody needs it”, which is untrue.

I don’t think it’s less readable in TS; and usually the props types are defined outside the component in a type/interface.

@max-prtsr nope, just the one that makes decisions here. an eslint rule is supposed to be opinionated - choices are for style, but any choice that causes bugs is indeed one that folks should be actively prevented from making.

@ljharb you’re basically selling your opinion as the only valid.

The bugs I’m concerned about are exclusively about mutating the props object, since that’s the one React looks at when rerendering.

I prefer to destructure it in the first line of function body, it doesn’t make my code worse and less robust obviously. There is prefer-destructuring rule allowing to avoid not destructured properties.

And what if I want to combine props with some other variables using spread operator and pass the result to child component? It’s very useful when child component has common properties with its parent (wrapper component).

The notice about multiline destructuring in signature is still actual.

That should suggest to you that “having 10-20 props” is what causes the readability problem, not “where they’re destructured”.

Either way, I think this:

function Foo({
  a,
  b,
  c,
  d,
  e,
  …
  z,
}) {

is pretty objectively more readable than this:

function Foo(props) {
  const {
    a,
    b,
    c,
    d,
    e,
    …
    z,
  } = props;
}) {

especially with long lists, because it duplicates the props reference, and the longer the list, the farther apart they are.

What I’m saying is that not all stylistic preferences are equal, and sometimes, a choice actively harms the maintainability of a codebase, and in these cases, it’s better for an eslint plugin (which by definition are meant to be opinionated) not to tacitly endorse the pattern by providing configuration for it.

You are welcome to modify the rule in your own plugin, and use that.

I don’t think it’s less readable in TS; and usually the props types are defined outside the component in a type/interface.

Maybe, but an option of using props and destructuring in the function body would be nice for me 😃