[Question] here is a breaking change with 7.25.1 or 7.25.0

[SoloJiang]

https://github.com/alibaba/rax/runs/3458857417

[SoloJiang]

7.24.0 is ok

[ljharb]

This isn’t a breaking change, it’s a bug fix. Those errors should have been reported the whole time.

[SoloJiang]

But you can see, I am not use bind in these files.

[ljharb]

https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-no-bind.md#no-bind-or-arrow-functions-in-jsx-props-reactjsx-no-bind says "No .bind() or Arrow Functions in JSX Props".

[ljharb]

ok yep, that looks like a bug, thanks for the link :-)

cc @p7g

[p7g]

To clarify, is there a bug in the implementation or this behaviour should just not be enabled by default?

[ljharb]

@p7g there's a bug in the implementation.

@SoloJiang we need the link to the files to remain, otherwise we won't be able to fix it. can you restore it? (it looks like you deleted it)

[redbugz]

This is impacting us as well. For the usage we are getting flagged with, the message is confusing or misleading. And I wonder if the rule is too strict or not.

We are not using .bind() or inline arrow functions, we are using a local function inside a functional component to capture state
in a handler. It is specifically mentioned in the new docs:
https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-no-bind.md#react-hooks

const Button = () => {
  const [text, setText] = useState("Before click");
  function onClick() {
    setText("After click");
  }
  return (
    <button type="button" onClick={onClick}>{text}</button>
  );
};

The error message is "JSX props should not use functions". Well functions are completely normal in JSX props for handlers and the rule page title specifically calls out bind and arrow functions. Going through the rule doc clarifies that what it really is complaining about is that inline, arrow, and local functions have potential performance implications. But in our experience, there are many times when generating a small function each render results in a tiny perf difference not generally noticeable for lesser used components and flows.

Wrapping this function in a useCallback does make the render more performant, but sometimes the perf difference doesn't seem to justify the additional code, at least to some.

If it's decided that local functions and arrow functions should always be wrapped in a useCallback, we can decide if we want to switch this to a warning from an error or follow the stricter practice. However, I think the error messages could definitely be improved to specifically call out the specific violation. We were very confused by both the error message and the title of the documentation as it didn't match our use case until we read that very specific subsection case.

[ljharb]

@redbugz the modern approach in SFCs is to use useCallback on all function props, and in class components, the modern approach has always been to use constructor-bound instance methods.

This rule should always have warned on these, and they're always preferable to avoid, so if you want to ignore this best practice, the proper thing to do is disable the rule.

I am always happy to improve error messages, and would welcome a PR that did so.

[redbugz]

Apparently this was a fairly common practice in our organization and our config was set to error on this rule, so suddenly many builds are broken. I understand your position and reasoning and now that it's clear how this rule will be applied, we can move forward. To get things moving again, we will switch to warning or pin back to 7.24.0, and then work to migrate the code forward. Thanks for quickly explaining your position.

[ljharb]

Note that we could revert it by default and put it behind an option - and if enough people report difficulty, we may. However, it's always nicer when we can roll forward and expose new users to the best experience by default.

[SoloJiang]

https://github.com/alibaba/rax/blob/master/packages/rax-server-renderer/src/__tests__/hooks.js#L121

Here you can find it.

[danielfinke]

You can sort of work around this for now by enabling allowFunctions in the config for the rule:

'react/jsx-no-bind': [
  1,
  {
    ignoreDOMComponents: false,
    ignoreRefs: false,
    allowArrowFunctions: false,
    allowFunctions: true,
    allowBind: false,
  },
],

although this will end up also allowing inline function expressions.

I think it's good that this workaround has been caught now. My team has been abusing this for too long 😄

However, it's always nicer when we can roll forward and expose new users to the best experience by default.

You have my support.

[Methuselah96]

To clarify, what is the bug? Shouldn't these cases error after #3048 if allowFunctions is set to fase?

[redbugz]

I'm getting just a little bit of pushback on this rule. I'm curious what you think about this post:

https://kentcdodds.com/blog/usememo-and-usecallback

Some say that using useCallback in all cases can in some cases be slightly worse for performance and certainly reduces readability somewhat and therefore should be used judiciously.

We'll adapt to whatever is decided. I was just curious if the above line of thinking is outdated, or has some merits but the lint rule will continue to encourage always using useCallback as the default, and the few cases where it is detrimental you would just ignore the rule with a comment, etc.

[ljharb]

I don't agree with that post, and prefer the rationale in https://attardi.org/why-we-memo-all-the-things/

In other words, yes, if you are fortunate enough to find one of the highly unlikely cases where it's detrimental, use a comment.

[ljharb]

This seems answered.