Eslint-plugin-react: destructuring-assignment for different component types

[HenriBeck]

I would like to propose an option for allowing different behaviors for a functional component and a class component for the destructuring-assignment rule.

[ljharb]

Can you elaborate, with code examples, and explain why you’d want it to be different?

[HenriBeck]

Personally, I like to destructure my props in a functional component, for the simple reason of passing additional extra props down to a specific child. Though I tend to not do that for class components.
Just having the option would be nice.
Of course, it would still allow for just a string as the first configuration. This would just add a few extra configuration options.

New rule configuration example:

module.exports = {
  rules: {
    'react/destructuring-assignment': ['error', {
      functional: 'always',
      class: 'never',
    }],
  },
};

The correct code for the configuration example:

function FunctionalComponent({ children }) {
  return children;
}

class Component extends React.Component {
  render() {
    return this.props.children;
  }
}

Incorrect code for the configuration example:

class Component extends React.Component {
  render() {
    const { children } = this.props;

    return children;
  }
}

[ljharb]

We'd need to also account for createClass components; but if we had all three, and all three defaulted to true (or, we had an "ignore" object with all three defaulting to "false", which is probably better) that could work.

[HenriBeck]

I would personally prefer rather allowing an object for more detailed configuration rather than turning the rule off for certain component types.

// Still support configuration for all three component types
module.exports = {
  rules: {
    'react/destructuring-assignment': ['error',  'always'],
  },
};

// Individual configuration for the component types
module.exports = {
  rules: {
    'react/destructuring-assignment': ['error', {
      functional: 'always',
      class: 'never',
      createClass: 'always',
    }],
  },
};

// Or allow for specific component overrides
module.exports = {
  rules: {
    'react/destructuring-assignment': ['error', 'always', {
      class: 'never',
    }],
  },
};

[ljharb]

Using always/never seems most appropriate here. Want to make a PR?

[HenriBeck]

Sure, I will start working on it next week. Which configuration should I implement?

[ljharb]

The always/never one you suggested, with "class", "createClass", and "SFC" as options, would be great.

[HenriBeck]

Also, I discovered this bug:
In both cases when destructuring is set to always it's not reporting an error.

function Baz({ ...rest }) {
  return (<div>{rest.children}</div>);
}


function Baz(properties) {
  return (<div>{properties.children}</div>);
}

Also is this intended?

class Comp extends React.Component {
  state = {
    animationName: this.props.active // Requires destructuring here when set to always
      ? 'fade-in'
      : null,
  };
}

Also when using something like react-jss it reports when using functions.

[ljharb]

The first one - { ...rest } - seems like a hack around the rule; I think we should change the rule to add an option that forbids accessing properties off of a rest prop. (in other words, rest.children would be an error).

The second one is a bug - the argument shouldn't need to be named "props" to be caught.

As for the third one, i'd say it's an antipattern anyways to have logic like that in a class field, because you'd also need duplicate logic in componentWillReceiveProps, so I think the proper fix for that is "move that to the constructor".

[HenriBeck]

Why would I not need to duplicate the logic in componentWillReceiveProps when I move the state initialization into the constructor?

I don't know if this is intended but for class methods, it's also reporting to destructure the props.

[ljharb]

You do, always (with state that depends on props), need to duplicate the logic in two places; at construction and in cWRP. I'm suggesting that it would be cleaner code to have the initialization-time part in the constructor rather than in a class field.

[HenriBeck]

Exactly, I personally prefer to have the initialization logic in class fields as it's not so much typing.
Either way, it shouldn't report an error there.

I will start working on a rewrite of the rule with the above-mentioned changes in mind, alright?

[ljharb]

I’m not convinced it shouldn’t report an error there; i tend to think this use case should not be permitted in a class field.

[HenriBeck]

There is no way of destructuring inside a class field so it definitely shouldn't report an error here.

[ljharb]

Correct; the error means you're not allowed to use a class field for this case.

[joggienl]

Just to be clear, does that mean that the example below (regardless what it does or implies it does) is not "allowed" and that I need to add the assignment for example to the constructor and in cWRP?

An approach like below is causing an error on one of my projects.

import React from 'react';
import PropTypes from 'prop-types';

class SomeComponent extends React.Component {
  // Next line triggers
  // "Must use destructuring context assignment  react/destructuring-assignment"
  example = this.context.someProp;

  static propTypes = {
    example: PropTypes.string.isRequired,
  };

  static contextTypes = {
    someProp: PropTypes.string.isRequired,
  };

  render() {
    const { example } = this.props;
    const { someProp } = this.context;

    return (
      <React.Fragment>
        someProp equals example?
        {someProp === example}
      </React.Fragment>
    );
  }
}

export default SomeComponent;

[kamronbatman]

I am having the same problem:

import React from 'react';
import PropTypes from 'prop-types';

class SomeComponent extends React.Component {
  state = {
    example: this.props.example, // triggers
  };

  static propTypes = {
    example: PropTypes.string.isRequired,
  };

  render() {
    ...
  }
}

export default SomeComponent;

Also triggers on:
state = someFunctionHere(this.props.example);

[ljharb]

@joggienl yes, that's what that means. you should use a constructor for that case.

@kamronbatman as should you. also note, your SomeComponent has a bug - it's setting state based on initial props, but you don't have a componentWillReceiveProps or getDerivedStateFromProps, so your component will be broken on rerender.

[kamronbatman]

It is a terribly simplified example, but I assumed you would understand what I meant.
I hate having to use a constructor if I don't need to, blah.
It would be nice to have an option to skip class body property assignments.

[ljharb]

I think that's a needed constructor. However, I do think adding this as an option also makes sense.

1740 might need to be merged first, however.

[HenriBeck]

1740 Is pretty much ready to go, maybe a last review before merging.

About the constructor, I already fixed this but adding an option seems like a good idea.

[kamronbatman]

@HenriBeck, #1740 looks good but it has merge conflicts.

[HenriBeck]

Yes, will resolve them tomorrow

[alexandernanberg]

There is now a separate issue for the issues with class properties #1875 (didn’t see this one until now)