react/destructuring-assignment: Add "minAttributes" option

[ThiefMaster]

This option would enable the rule only if at least n values are accessed. For example, with minAttributes=2 this would be OK:

class Foo extends React.PureComponent {
  render() {
    return <div>{this.props.foo}</div>;
  }
}

while this would trigger the rule's warning:

class Foo extends React.PureComponent {
  render() {
    return <div>{this.props.foo} {this.props.bar}</div>;
  }
}

The idea for this option is that especially in small components that only use one single property it's more convenient to not have to use destructuring (which does add one extra line of code!)

I wouldn't mind creating a PR to add this option if it is likely to get merged.

[ljharb]

Convenience isn't the priority; destructuring the props serves many useful purposes: for instance, for function props, it ensures they're not called with the this value as the props object.

Optimize for reading, not writing: in other words, convenience when writing code does not matter in the face of improving the readability of the code.

(Separately, a component that small should be an SFC, not a class-based component, and neither of those two components likely have any performance benefit by being a PureComponent)

[pferreir]

Other rules allow for options that most people will consider unorthodox (e.g. allowAllCaps in jsx-pascal-case). Would this option be that unanimously wrong that it wouldn't be worth having?
@ljharb I see your point and I fully agree with it. Still, I cannot help but be bothered by stuff like:

const {user} = this.state;
doWhatever(user);

If we're talking of readability, I believe most programmers out there would understand this.state.user better.

[ljharb]

@pferreir i agree that in the tiny percentage of cases where you're using a single prop or state value, it's slightly awkward to have to write the destructuring - but in the cause of minimizing future diff churn, when inevitably someone adds a second prop or state field, the change is much more minimal when they'd destructured in the first place.

[ThiefMaster]

Except that there are components where it's very unlikely they'll have more than one prop in the future

[ljharb]

@ThiefMaster those sound like they should be SFCs, where they can be destructured in the component's arguments.

[UchihaVeha]

const SnackBarConnected = props => props.message && <SnackBar {...props} />; 

const SnackBarConnected = ({ message, ...props }) => 
 message && <SnackBar {...props} message={message} />;

@ljharb Do you think variant 2 is looking better than 1?

[ljharb]

@UchihaVeha considering that both will throw a React error when message is undefined, i think neither look particularly good. Here’s how i would write it:

function SnackBarConnected(props) {
  const { message } = props;
  if (!message) { return null; }
  return <SnackBar {...props} />;
}

[UchihaVeha]

@ljharb You did not understand me, what if I need to use some prop in current component and pass this prop to the child with other props and using the arrow function?
1.

const myComponent = props => 
  <div>
    <span>{props.user.email}</span>
    <Account {...props} />
  </div>

const myComponent = ({user, ...props}) => 
  <div>
    <span>{user.email}</span>
    <Account {...props} user={user} />
  </div>

[ljharb]

@UchihaVeha in both cases, you should use an explicit return, so that you can add a destructuring statement above:

const myComponent = (props) => {
  const { user: { email } = props;
  return (
    <div>
      <span>{props.user.email}</span>
      <Account {...props} />
    </div>
  );
};

const myComponent = ({ user, ...props }) => {
  const { email } = user;
  return (
    <div>
      <span>{email}</span>
      <Account {...props} user={user} />
    </div>
  );
};

[dolsem]

@ljharb What do you think of this

shouldComponentUpdate(nextProps) {
  return this.props.uri !== nextProps.uri;
}

vs this

shouldComponentUpdate(nextProps) {
  const { uri } = this.props;
  return uri !== nextProps.uri;
}

?

[ljharb]

@dolsem I definitely prefer the second one, altho I think you've identified a current problematic case - specifically that this:

shouldComponentUpdate({ uri }) {
  return this.props.uri !== uri;
}

will still warn on this.props.uri - I think the rule needs an option that toggles whether it forces destructuring when a renaming would otherwise be necessary.

[dolsem]

@ljharb I like neither my second version nor the version you provided, because I think accessing uri directly without restructuring makes more evident what's happening conceptually. If you isolate the return line from the rest of the code, it's much clearer what's going on in my first version.

In the example you provided the problem is easily avoidable by not destructuring nextProps (Captain Obvious here). Cause if you do it this way, it's not as clear what argument exactly the method accepts (nextProps is a very descriptive name), and generally speaking, I feel like destructuring gets abused way too often, with cases just like this, where you don't really need it.

I think it's almost always best to destructure this.props, but in my previous example the tradeoff is not in its favor, unless I'm missing something. Personally, I would prefer to have an option to disable the warning when the prop you're accessing is specifically being used to compare it with a variable or prop with the same name. Or, a less sophisticated alternative would be to have a config option that just disables the rule inside shouldComponentUpdate.

[dandonahoe]

Another scenario where this would be useful is when there's only one mention of this.props in a function. For instance

Too verbose for simple functions

handleThing = (evt, id) => {
  const { dispatchOpenDialog } = this.props;

  dispatchOpenDialog(DIALOG_ID, {
    id
  });
}

Cleaner solution when this.props is only used a single time. Having a linter option for minAttributes: 1 would be very nice to have

handleThingSimple = (evt, id) =>
  this.props.dispatchOpenDialog(DIALOG_ID, {
    id
  })

[ljharb]

This actually seems like a duplicate of #1629.

[ljharb]

@dandonahoe that example is exactly the worst hazard - this.props is now the receiver (the this value) of the prop function, and you’ve just exposed yourself to a much larger class of bugs by implicitly passing the props object elsewhere.

Even if there was an option like this (which I’m not inclined to add) it would never allow you to avoid destructuring a function prop.

[dandonahoe]

@ljharb Thanks for the response.

Correct me if I'm wrong, but wouldn't this be fine since the only value being passed back from handleThingSimple is the return value from dispatchOpenDialog? It's not passing this.props anywhere, nor would that be accessible outside the scope of this code.

If I'm missing something, i have a good chunk of code to go back and refactor.... which if fine because I'd rather get it right.

[ljharb]

You’re missing that in JavaScript, the object the method is called in gets implicitly passed as the this value to the function.

[AlicanC]

Not having this was problematic for us in this case:

class MyComponent extends Component {
  // How it was (good)
  onButtonPress = () => {
    analytics.track('buttonPressed');
    this.props.doThing();
  }

  // How it become (bad)
  onButtonPress = () => {
    analytics.track('buttonPressed');
    const { doThing } = this.props;
    doThing();
  }

  // What I dream of (off topic, but I had to mention 😁)
  @track('buttonPressed')
  onButtonPress = () => this.props.doThing();
}

[ljharb]

@AlicanC that's specifically the footgun that must be avoided - this.props.anything() is an antipattern and must never appear anywhere in code. That's the primary reason I'm unlikely to ever agree to add a "count" to this rule.

[dandonahoe]

@AlicanC I asked a similar question above, not realizing the nuance about the “this” scope. The followup posts explain

#1731 (comment)

[AlicanC]

@dandonahoe IMO your problem is better solved by something like #1968.

@ljharb IMO the footgun is not this.props.func(), but this itself. this should be typed and checked by Flow/TS. Until then, I'd rather not make my team write tons of extra chars for this very minimal possibility.

[ljharb]

It’s not “tons”, and this is a javascript linter, not a Flow or TS linter. Are you honestly trying to say tho that you strictly type your functions to have no this value, and that the type system thus doesn’t force the same code?

[AlicanC]

All I mean is that this is a type issue. ESLint can't prevent me calling a function with wrong arguments and it can't prevent me calling a function with wrong this. It doesn't know types so it doesn't know what is right or what is wrong. We use Flow and it doesn't know it either, but if one of these tools is going to solve this issue properly it's Flow (or TS) and not ESLint.

If we go back to this example:

class MyComponent extends Component {
  onButtonPress = () => this.props.doThing();
}

To achieve this we type all of our callbacks () => mixed so the return value of doThing() can not be used accidentally. We also heavily avoid using classes or this so we are covering that too. What we have done is get rid of the foot so this footgun has nothing to shoot at.

If we ever wanted to be 100% sure about this, we would need something like facebook/flow#452 and not some ESLint rule that says "I don't know if what you are doing is right or wrong, so I will disallow it completely, including the valid cases". This is sloppy and is not something we want.

I can also assure you that I am not one of those "Let's rename const to cnt so we save two chars." people on ES Discuss. As I've said on #1968, this ESLint rule was already one of our verbal rules, but I ended up seeing hundreds of violations from other developers in my team (even after being corrected in reviews multiple times) and I had to let it go. These rules are enforced on actual humans and your humans might be okay with this, but our humans are resisting it, so I like the idea of enforcing this rule with ['error', 'always', { selectors: ["FunctionBody"], maxAmount: 1 }], but not with ['error', 'always']. We are trying to be a fun-to-work-at startup and not a we-get-bored-in-cubicles company. "This bad thing might happen some time in the future so you have to code like this all the time, every day." is just not fit for our culture so I rather increase my reviewing time a little bit to save a lot more time for the developers.

[ljharb]

eslint certainly can prevent you from calling props functions with the props object as the this, unintentionally, and this rule helps with that. Types aren't necessary for "right and wrong" nor do they exhaustively determine that.

Using types to paper over incorrect engineering - like returning a value from onButtonPress when you have no intent to use it - is not a good use of types. That must be onButtonPress = () => { this.props.doThing(); } and if your types aren't enforcing that, they're not helping you.

If you need to turn off a rule because your developers refuse to learn best practices, you're welcome to do that - but that doesn't mean the rule should be weakened.

I'd certainly be open to making this rule more autofixable, however, if that would ease the burden.

[AlicanC]

What does this have to do with react/destructuring-assignment though? Why not make a separate rule for disallowing props.method() and let this rule be used for styling purposes?

I understand that you have your best practices, but we have our own too. To us, this isn't a case of "developers refusing to learn best practices". Our developers do not refuse to learn best practices, our developers are under-experienced and I don't want to throw hard-to-follow rules at them.

We've had people quit because the job was too hard. I've had people contacting me asking for help because they were losing their minds trying to figure something out. This isn't about reaching the best code quality, it's about reaching the best code quality without making people shout at their laptops and go to sleep with impostor syndrome. We even have developers who don't have to fix ESLint/Flow/Jest errors, I checkout their branches and fix the errors for them.

Now, if we could have this rule to be configurable as { selectors: ["FunctionBody"], maxAmount: 1 } it would help our developers and reduce the time I spend on reviews. Our code would be more maintainable than before, and you could still configure it the way you want.

[ljharb]

@AlicanC i'd be open to modifying the rule to add an option that specifically only enforced destructuring on function invocations, and also to a PR that adds autofixing, which should address the "hard" complaint.

[yardenhochman]

Hey ljharb I was running into this issue often in class methods and I was wondering about it.
Any reason why:

login = () => {
      const { loginUser } = this.props;
      loginUser(this.state);
    }

is better then
login = () => this.props.login(this.state)

specifically when it comes to class methods, I feel like the latter is much more readable and approachable

[ljharb]

@yardenhochman separately, you should never put arrow functions in class fields - but why it's better is that in your second example, the login prop gets this.props as its this value, which unnecessarily exposes your props object to consumer code.

[alexzherdev]

I'm wondering - in the parent component that provides the function prop, that function would normally be already bound somehow, right? Even if it's via an arrow function, but ideally with bind.

class Parent extends React.Component {
  constructor(props) {
    super(props);
    this.handleLogin = this.handleLogin.bind(this);
  }
  handleLogin() { this.blah(); }
  render() {
    return <Child login={this.handleLogin} />;
  }
}

If that's the case, then this.props would never be passed as this, would it?
FWIW that's the reason I've never seen this issue occur in my code, even though I don't always destructure function props before calling.

[ljharb]

@alexzherdev certainly it could already be bound, or be an arrow function, or simply not use this. However, that’s impossible to know inside the component in question.

[rdsedmundo]

+1 on that.

I'm with @yardenhochman. I really prefer the 2nd code as this destruct is completely unnecessary IMO. I'd like to see a minAttributes option as well.

[yashaRO]

I think minAttributes would be the best middleground, but I also totally understand where @ljharb coming from.

[malyzeli]

@dandonahoe that example is exactly the worst hazard - this.props is now the receiver (the this value) of the prop function, and you’ve just exposed yourself to a much larger class of bugs by implicitly passing the props object elsewhere.

Even if there was an option like this (which I’m not inclined to add) it would never allow you to avoid destructuring a function prop.

Can you please elaborate (with a bit of code examples) or link us to some explanatory articles about this "much larger class of bugs"?

This is the first time I see it and unfortunately I have this syntax (see below) all around my codebase (usually disabling destructuring-assignment rule on individual file level) in many projects.

I really want to understand the problem so I can fix it properly (and of course I will stop disabling the rule, haha).

Thanks for your time!

Example:

class App extends Component {
  handleLogin = () => this.props.loginUser(...)
  // ...
}

[ljharb]

@malyzeli the loginUser prop there could mutate its this - changing your props, and making your component rendering nondeterministic.

[malyzeli]

@ljharb well, it's true. I didn't realize that because most of my codebase is purely functional - I never mutate function input parameters and the only place I use this keyword is in React.Component classes.

I think it's questionable if you need to be guarded by destructuring-assignment rule when you respect no-param-reassign. But that's of course something that must be considered individually, and each project may configure this rule according to their needs.

I made an example code for those who want to see the difference, it's available here:
https://gist.github.com/malyzeli/0476d96259fe97f5dfe25d8b5abf09ec

[aldeka]

Strongly encourage a way to turn this rule off. It's fine if you think always destructuring or never destructuring are better code styles, but many people disagree! You accommodate many other options and variants in eslint (tab/spaces, anyone?)--why the intransigence here?

There are plenty of cases where you're only ever using a single prop or state field and now after updating you've suddenly got a giant angry warning all over it.

There are many cases where you have short class methods, like

toggleRecurringPaymentsChecked = () => {
    this.setState({ recurringPaymentsChecked: !this.state.recurringPaymentsChecked });
}

which passed before and fail now.

There are plenty of cases, especially in larger/container components, where having many naked, destructured variables running around makes it harder to tell what each represents and where each of them originated from--is it props, state, a constant, or a computed value? Choosing to include the this.props or not is actually purposeful and informative sometimes.

These patterns might not be to your taste, and that's fine--enforce your rules on your projects!--but they are to our taste. The addition of this rule to eslint with only ALWAYS destructuring or NEVER destructuring is making it harder, not easier, to enforce our code style. :/

[ljharb]

@aldeka because it's not a style choice; it's a correctness choice, which always trumps style.

[kmenshov]

There is always a balance between good practices and convenience. In many-many-many cases being unable to limit this rule, lets say, starting from 2 attributes or more only leads to it being disabled at all. Which is kind of sad.

So, instead of inflicting good practices it is being excluded from usage at all, which contradicts the point completely.

[bohdanbirdie]

Upvote for this thread
I personally see a ton of frustration by being forced to destruct even when I only use a single property

[ljharb]

Given the inherent dangers in this.props.foo() (it exposes this.props to foo as the receiver), it's pretty important to always destructure every prop, without exception.

I understand the argument that it might cause the rule to be disabled, but I don't think that's worth making it so easy to do the wrong thing.