New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
jsx-no-bind should complain when using an arrow function #3114
Comments
Btw forcing useCallback/useMemo is very controversial:
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). |
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 |
This seems very related and/or a duplicate of #2901. |
If a rule causes controversy, would it be better to add an option to allow an exception (something like
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 |
That complexity and burden is absolutely worth the tradeoff - there's really no argument that holds up to avoid maximal memoization. |
@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:
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 🙂 |
@JoseLion imo it's because React can't do that - the architecture of the JS language prevents it from doing it properly. |
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 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 |
So, first - in the OP, it's a bug that @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) |
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 😁 |
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? |
@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 From my side, I think it's ok to leave this rule as is 🙂 |
Sounds like this is resolved, so I'll close it. If there's any future questions or issues with this rule, please feel free to file a new issue! |
With eslint-plugin-react 7.26.1:
Array function like regular function should display an error.
The text was updated successfully, but these errors were encountered: