jsx-no-bind - active for custom components only

[janmarek]

Hi.

I'd like to use this rule to avoid problem with shouldComponentUpdate mentioned in this article https://medium.com/@esamatti/react-js-pure-render-performance-anti-pattern-fb88c101332f#.i77y134gm (section Functions create identities too).

But this is only a problem on custom components. HTML elements like or so don't have shouldComponentUpdate, so binding functions is much smaller problem there.

Currently I have 126 jsx-no-bind warnings in my app and I'd like to fix the important ones first. It'd nice to be able to turn this check on custom components only (their names start with a capital letter).

Thank you

[mindjuice]

This is something I'd like to see too. Would a PR for this be welcome? If so, I can work on it.

[ljharb]

@janmarek the issue is that .bind is slow, and creating a new one on every render is going to be just as slow with HTML elements as with anything else.

[mindjuice]

The performance of bind() is not the issue IMO. It's that passing bind() as a prop to a custom component creates a new instance of the function each time, thereby eliminating the effectiveness of shouldComponentUpdate().

This is obviously very undesirable for custom components, but for a <div>, you're already in the process of rendering anyway, and there is no shouldComponentUpdate() function to worry about, so it seems reasonable to allow an exception for this case (opt-in, of course).

If I have a list of a dozen or so options that gets rendered infrequently (and I often do), I don't much care about the overhead of bind(), but I don't really want to create a new component for each thing in each of the various lists I have. Even if I went with a new component (which I sometimes do where it makes sense), there would be overhead in calling the constructor of that component and passing props to it. This would, at least partially, offset the overhead of bind().

Right now, my options are to disable this rule on the lines where I use bind() or disable the rule entirely. I'd rather be able to set a parameter in eslint like ignoreHtmlTags=true (or whatever you want to call it).

[ljharb]

Why are you using bind instead of an arrow function though?

[mindjuice]

To pass additional parameters to an onClick handler so it knows which list item was clicked (e.g,. onClick={this.handleClick.bind(this, index, group)})

If you know of a better way to do that, I'd love to hear it.

[ljharb]

@mindjuice onClick={e => this.handleClick(index, group, e)}

[janmarek]

Bind and arrow functions have the same effect here. Both of them create a new function. And both are ok with HTML nodes but not with custom components because of that shouldComponentUpdate.

[ljharb]

@janmarek "a new function" isn't the problem, Function.prototype.bind is the problem. That is what's slow.

[janmarek]

I guess bind has two problems and arrow function has only one then.

[mindjuice]

The arrow function will not work if you are using a plain loop to create your <div>s, because the index and group are references to the closure variables, and by the time handleClick() is called, index and group would be at their final values.

In my case we're using map(), so it should be fine, and I'll change to that for now.

I still believe that an additional option, as I described above, would be useful. eslint's job isn't to tell developers how to write their code, but to give them tools with the flexibility to let them enforce whatever rules they choose.

[mindjuice]

@ljharb bind() may be slow, but passing a new function as a prop every time to an expensive-to-render component is going to be much more of a performance issue, since the shouldComponentUpdate optimization is made useless.

[ljharb]

This rule now has the ignoreDOMComponents option, as well as ignoreRefs, which covers this issue, added in #1868.