Eslint-plugin-react: maxCount option on `destructuring-assignment` rule?

[mockdeep]

I'm implementing the destructuring-assignment rule, and generally I like it, however in cases where I only reference props once inside a function it feels like overkill. It would be nice to be able to configure it to require destructuring after a certain max number of prop references:

// maxCount: 1
// this is okay
const MyComponent = (props) => {
  return (<div id={props.id} />)
};

// this is a warning
const MyComponent = (props) => {
  return (<div id={props.id} name={props.name} />)
};

Might also be nice to have a warning for the inverse as well:

// this is a warning to not destructure:
const MyComponent = (props) => {
  const {id} = props;
  return (<div id={id} />)
};

// this should maybe be okay, though?
const MyComponent = ({id}) => {
  return (<div id={id} />)
};

(Side note, this rule actually seems more broadly applicable to JS in general, would be nice to have it recognize any place where destructuring could clean things up.)

[ljharb]

i don’t think it’s overkill; i think the arguments for doing it 5 times are the same for doing it once.

[mockdeep]

I guess it depends on your rationale. For me it's about minimizing repetitive and/or unnecessary code, and from my perspective it's a clear benefit when we eliminate 3 or more repetitions, it's a little borderline with 2, and at 1 it kind of feels like the tail wagging the dog.

[ljharb]

Minimizing repetitive code is helpful, but other benefits include using a variable instead of an object access; preventing the case of props.foo() (where the this value of the "foo" prop becomes the entire props object); ensuring that the linter is able to mark all used props as used at the top of the function/component; and preventing the massive antipattern of passing around the props or state objects.

[mockdeep]

These sound like some valid concerns, though it doesn't seem like the destructuring-assignment rule is necessarily the best tool to handle them.

For the this binding issue, with or without the rule I still end up with a similar runtime error that I need to debug. What would be amazing is to have a rule that checks on the parent if we're passing this.someMethod as a prop, and if that method refers to this, and if said method is not bound. It wouldn't cover cases where you're passing a method from another object, but would still probably cover better than 90% of cases where the binding even matters, at least for me.

For the case of passing props or state, I don't think the current implementation of this rule really covers that. It also seems like a good case for another rule entirely that explicitly disallows it. The idea for a no-spread-props rule at least partially covers it, though.

I'm not sure I understand your point about object access. Is it just an aesthetic preference to avoid referencing the object, or is there something else I'm missing?

[Dzieni]

Thumbs up for this idea, it would be really helpful. Now I need to disable this rule since it's just officious in many cases.

[ljharb]

@mockdeep if you reference props.foo more than once in the function, that has overhead that's avoided if you extract it once and reference the extracted value.

[Dzieni]

@ljharb In such a short function you rarely reference a property more than once.

When the maxCount equals 1, it's a good compromise between optimising object referencing and avoiding to write unnecessary destructuring assignments.

[ljharb]

@Dzieni certainly if you only reference it once, there’s no object lookup benefit. But the other benefits still apply.

[mockdeep]

@ljharb are there benchmarks on the overhead of object lookup? It seems like that's the sort of thing that would have negligible performance impact, even at pretty large scale.

[ljharb]

Yeah, it probably is negligible - I don't have benchmarks offhand. In normal cases, I'd say "getters", and that's resoundingly significant, but for props/state (objects react creates) that's a nonissue.

Either way, the other benefits I mentioned apply :-)

[SteveLillis]

The other thing to keep in mind is that if you allow inlining of one prop usage, you'll have messier Pull Requests for the iteration of software that adds a second prop usage. That said, flexibility in the rule would be handy to allow the developer to decide what they prefer.

[ljharb]

It sounds like almost all the utility of this requested option is for "i want to ignore the rule when there's only one prop" - would that be accurate?