jsx-no-bind should complain when using an arrow function

[tkrotoff]

With eslint-plugin-react 7.26.1:

function OtherComponent({ onChange }) {
  return <input onChange={onChange} />;
}

function MyComponent_arrow() {
  // No error
  const doStuff = () => { ... };

  return <OtherComponent onChange={doStuff} />;
}

function MyComponent_function() {
  // Error "JSX props should not use functions  react/jsx-no-bind"
  function doStuff() { ... }

  return <OtherComponent onChange={doStuff} />;
}

Array function like regular function should display an error.

[tkrotoff]

Btw forcing useCallback/useMemo is very controversial:

• https://dmitripavlutin.com/dont-overuse-react-usecallback/
• https://kentcdodds.com/blog/usememo-and-usecallback
  • https://www.reddit.com/r/reactjs/comments/bzwin2/kent_c_dodds_when_to_usememo_and_usecallback/
• https://overreacted.io/before-you-memo/
• https://en.wikipedia.org/wiki/Program_optimization#When_to_optimize: "premature optimization is the root of all evil" (Donald Knuth)
• https://attardi.org/why-we-memo-all-the-things/

Even the new React documentation don't use useCallback: https://beta.reactjs.org/learn

I would split this rule: jsx-no-bind-classes (enabled by default) and jsx-no-bind-hooks (disabled by default).

[ljharb]

https://attardi.org/why-we-memo-all-the-things/ is in favor of forcing it, to be clear.

If you don't like a rule, you can turn it off, but controversy doesn't mean both sides are equally correct.

Splitting a rule like you suggest is a lot of churn and a breaking change, and is definitely not worth the effort here.

---

I agree that doStuff should error in both of those last two examples.

[fgblomqvist]

This seems very related and/or a duplicate of #2901.

[JoseLion]

If a rule causes controversy, would it be better to add an option to allow an exception (something like allowLocalDeclaration)?

• Enforcing memorization might not be wrong, but it does add complexity and an extra burden on developers. Maintaining the callback dependency array is another thing to consider.
• Enabling the options allowArrowFunctions and/or allowFunctions leave things too open. It allows inline anonymous functions in props, which works against pure component rendering. It's better to avoid them to prevent the component from re-rendering more often than necessary.
• It also makes it much harder to migrate from tslint to @typescript-eslint, given the jsx-no-lambda rule in tslint-react checks for inline functions, but it does allow local declarations. At the moment we're left with only 2 options: 1) Enable allowArrowFunctions which may be undesirable because it allows inline functions, or 2) make a possibly huge refactor to force memo everywhere.

IMHO, I think it shouldn't be about forcing one side or the other, but rather enabling more use cases in different code bases. I think adding an allowLocalDeclaration option can accomplish that 🙂

[ljharb]

That complexity and burden is absolutely worth the tradeoff - there's really no argument that holds up to avoid maximal memoization.

[JoseLion]

@ljharb I'm almost on the same page. But if there's no argument against maximal memorization, the bigger question is, why doesn't React do that? Why do we need to add all that code when React could automatically memorize everything for us?

The answer to that question is hidden in https://attardi.org/why-we-memo-all-the-things/, right where it cites Christopher Chedeau on Twitter. The reply to Dan Abramov in the article is incomplete, and in its second part, we find our answer:

The reason we don't want to do it for React is that 1) correctness is not guaranteed for everything because people can mutate things. 2) the cost of doing the comparison can become prohibitive if we're comparing big data-structures or rendering small components.

Christopher Chedeau.

Anyways, I think a lot of codebases and teams could benefit from this rule and full memoization. There are some great points in https://attardi.org/why-we-memo-all-the-things/, I'm personally almost convinced, except for the 2 reasons above. But giving the flexibility to others to handle it in a different way would also be a great addition to this rule 🙂

[ljharb]

@JoseLion imo it's because React can't do that - the architecture of the JS language prevents it from doing it properly.

[JoseLion]

Yeah maybe, but still the reasons mentioned by Christopher Chedeau apply to the rule 🙂

I think a point to consider in this context though is that using the rule with the changes on #3048 on codebases that don't use memo, useMemo, and useCallback everywhere will require a big refactor to memorize everything. That can be so risky and time-consuming (expensive), that folks may prefer to go with the allowArrowFunctions option as a scape-hatch. The proposed allowLocalDeclaration option will at least prevent the use of anonymous inline functions in props.

Or maybe another option could be to have a separate rule that prevents the use of inline arrow functions on JSX props. That way we don't add more complexity to jsx-no-bind and the intention becomes clearer too. Wdyt? 🙂

[ljharb]

So, first - in the OP, it's a bug that doStuff is not erroring in both of the last two examples. That should be fixed.

@JoseLion can you reiterate exactly what code is being warned that you want to not warn? (or, which code is not being warned on that you want TO warn)

[JoseLion]

Got it! So basically, the idea is to be able to ban the use of inline functions on component props. So even if I have the following rule configured:

{
  "jsx-no-bind": ["error", { "allowArrowFunctions": true }]
}

It would be desirable to ban the following:

function MyComponent() {
  const handleBlur = () => { /* do stuff */ };

  return (
    <input
      name="foo"
      onChange={event => { /* do stuff */ }} // ❌ We want to be able to always warn about this (inline functions)
      onBlur={handleBlur} // ✅ This is ok, but it depends on how `jsx-no-bind` is configured (function reference)
    />
  );
}

If not for optimization, it could be considered good practice so the transition to memoization is easier and less disruptive. I hope this is helpful! I'll be happy to help out with a PR too 🙂

PS: Found an interesting RFC about React itself handling the memoization (in a way). It has some time under discussion, but hopefully, we may have something along those lines some day 😁
https://github.com/reactjs/rfcs/blob/useevent/text/0000-useevent.md

[ljharb]

I would consider both of those props (onChange and onBlur) to be identical - they're both effectively an inline arrow function. I do hear you that it's a gateway step to proper memoization, but why not just make the full change?

[JoseLion]

@ljharb, sorry it took so long to reply. I've been a little busy, and to be honest, I took some time to put this rule into practice too. I agree with you that proper memoization is the way to go, especially to help prevent breaking optimization on third-party libraries, which is code we usually don't control over 🙂

I see now that my argument about "a gateway step to proper memoization" will not be beneficial in the long term, so there's no reason not to use useCallback(..) instead. My other argument based on leaving optimization aside was more inclined to code styling, but now I think it's not worth adding more options based on style, especially if they can cause more trouble instead 😅

From my side, I think it's ok to leave this rule as is 🙂

[JoseLion]

In terms of the reported bug, I haven't been able to reproduce the issue on version 7.32.2