jsx-eslint / eslint-plugin-react

React-specific linting rules for ESLint
MIT License
9k stars 2.77k forks source link

Create a new rule or update jsx-no-bind to work with react hooks and jsx #2901

Open sbrighiu opened 3 years ago

sbrighiu commented 3 years ago

I am looking for a lint rule that helps me enforce not declaring inline functions (function type is irrelevant) inside jsx code. React hooks don't need binding anymore soo... i'm not sure if including this inside jsx-no-bind would be fair..

React Native basic example below: Screen Shot 2021-01-13 at 20 01 31

This is a something profiling nerds need and I a recommended way of handling functions when jsx is concerned. One possible option available could be to allow one liners to still exist (but i don't see a reason why other than laziness).

sbrighiu commented 3 years ago

Discussion may be related to https://github.com/yannickcr/eslint-plugin-react/issues/1761

ljharb commented 3 years ago

In an SFC, the proper solution here (and only when functions are passed into custom elements, not HTML elements) is to cache the function at module level, or use the useCallback hook.

Given the latter, this seems like something eslint-plugin-react-hooks should be warning about (if it doesn't already). Have you looked into that plugin's rules?

sbrighiu commented 3 years ago

Hmm. Let me do a quick check.

sbrighiu commented 3 years ago

I'm tired :) I actually checked.

react-hooks only enforces https://reactjs.org/docs/hooks-rules.html a couple of rules related to hooks themselves.

It has no real business with how you write jsx.

sbrighiu commented 3 years ago

I feel like this is a good blog post on why it is not really ok to useCallback everywhere post. There are longer explanations, but this one should suffice. Most of the functions written can be shortened and abstracted away such as they don't really impose a threat to performance, like a navigate() call to a screen.

ljharb commented 3 years ago

and here’s a good one on why you should always use it: https://attardi.org/why-we-memo-all-the-things/

sbrighiu commented 3 years ago

I don't want to be rude or anything .. but not every developer is a senior developer and understands or can maintain that standard. Also not all of us work on new projects and have unlimited time on our hands :). I'm no denying the benefits, it's just that we are not talking about the 10% that actually keeps their code in shape 24/7, we are talking about everyone that writes code.

Some, like me, have pipelines that run the linter at commit (for example). In my case i want to add this (as a warning) to make it aware to my team that it is good practice to at least not inline functions and let them figure out why this is not a good idea and read articles like the one you posted.

Different teams have different priorities and different skills. Joining a new project and a new team and enforcing this would cripple productivity and is never a good idea.

ljharb commented 3 years ago

This is all true. My experience, tho, tells me that linter warnings are useless; only things that fail the build end up being enforced.

A lint rule to always use useCallback and React.memo, especially if it autofixed, would be very interesting tho, and seems like it would mitigate the productivity concerns you have.

sbrighiu commented 3 years ago

Hehe, you would be surprised at how fast that can mess up performance and UI updates across the board :)) (android is really slow :)) especially when the function code depends on a global context state.

Yeah, warnings don’t necessarily do much and yes enforcing it would be the way to go for performance sake. For now i just want a way to highlight it in code and make some PR automated commits based on linting. Most files currently have at least 1-2 inline functions that do too much already.


Stefan Brighiu Software Engineer @ AB4 Systems Phone: +40 744 581 921 Web: http://ab4.systems On 13 Jan 2021, 21:45 +0200, Jordan Harband notifications@github.com, wrote:

This is all true. My experience, tho, tells me that linter warnings are useless; only things that fail the build end up being enforced. A lint rule to always use useCallback and React.memo, especially if it autofixed, would be very interesting tho, and seems like it would mitigate the productivity concerns you have. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

KonstantinZhukovskij commented 3 years ago

I agree, I'm also looking for something similar

ljharb commented 2 years ago

A lint rule to always use useCallback and React.memo, especially if it autofixed, would be very interesting tho, and seems like it would mitigate the productivity concerns you have.

VGamezz19 commented 2 years ago

so, any update?

dzek69 commented 1 year ago

I was looking for some other thing related to jsx-no-bind and I can't understand what's the problem.

I don't see hooks in the example in the OP comment.

Anyway - if you use useCallback then the issue is gone. This is the correct way to solve the issue for 99% cases (excluding refs). This is already explained in the docs. If you don't want to use useCallback you have a plenty of exclusions (see options in documentation).

If that's still not satisfying the rule is just not for you. There is nothing else in between.

allow one liners to still exist

That's defeating the whole idea of the rule. It's not about lenghty callbacks, it's about function identity changes causing unneccessary re-renders.