Allow jsx-no-bind and jsx-no-lambda to ignore DOMComponents

[koba04]

I think function binding and anonymous functions with DOMComponent would be fine, the cost to create a function each time is not significant. So it makes sense to add a option to ignore DOMComponents.

eslint-plugin-react has added this option as https://github.com/yannickcr/eslint-plugin-react/issues/1238

[palantirtech]

Thanks for your interest in palantir/tslint-react, @koba04! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

[benny-medflyt]

What's the status on this? This is a must-have feature @koba04

[koba04]

@benny-medflyt The last commit of this repo is Jun 2018 and we can't see any activities except a CLA bot. So I guess this repo seems to be no longer maintained.

[benny-medflyt]

So I guess this repo seems to be no longer maintained.

@adidahiya @JKillian Any comment?

[benny-medflyt]

Thinking about this some more, the test whether to allow/disallow anonymous functions on props should not be whether or not the component is a DOMCompoment. Instead, the test should be whether the component inherits from React.PureComponent. This is something where tslint has an advantage over eslint since it can utilize the type information to determine this.

[adidahiya]

Can someone provide some more documentation or a fleshed out proposal of the overall logic here, possibly with some performance metrics? I'm not sure I understand all the details around this change via the eslint rule you linked... why exactly are DOMComponents ok?

[benny-medflyt]

I can try to explain it the way I understand it.

Let's say I have 2 react components:

function MyComponent1(props: {}) {
    return (
        <div>
            <ListView
                onClick={() => alert("clicked")}
                items={myItems}
            />
        </div>
    );
}

function MyComponent2(props: {}) {
    return (
        <div>
            <button
                onClick={() => alert("clicked")}
                disabled={false}
            />
        </div>
    );
}

When "MyComponent1" is re-rendered, then the ListView will also be considered for re-rendering. If ListView inherits from PureComponent (or implements shouldComponentUpdate) then it will compare the previous props it had to the new props. But since we are using an arrow function for onClick then the function will always be different, so the ListView will always re-render, even though it has not really changed. This re-rendering of the ListView might be very expensive and cause the app to be slow. This is because the actual "render" function of the ListView might perform expensive computations, and/or because ListView might have many children that now also need to be re-rendered. The solution: don't use an arrow function for the onClick prop. This way, the before- and after- props will be identical, and the PureComponent shouldComponentUpdate algorithm will detect this and prevent ListView from being re-rendered.

Now the story of "MyComponent2" is similar, but different.

When "MyComponent2" is re-rendered, then the button will also be considered for re-rendering. But since "button" is a DOM element, it will always be re-rendered, no matter what. DOM elements cannot inherit from PureComponent and cannot have a shouldComponentUpdate. So it makes no difference if we use an arrow function or not, the button will be "re-rendered" no matter what.

Note: There may be a tiny performance increase by using a non-arrow function with the button, since react will see that the onClick handler is unchanged, so it will be able to skip un-registering the previous event handler on the DOM node and registering a new event handler. But this is not the performance concern we are worried about when talking about arrow functions in react.

Thinking about it some more, I the actual distinction we care about for this tslint rule, is not whether or not the React component is a DOM node or not a DOM node. I think what we are really interested in is whether the React component inherits from PureComponent or not (and DOM components fall into the category of those not inheriting from PureComponent). Sometimes you have a fancy custom "Button" element that is just a small wrapper around <button>, and your "Button" is not a PureComponent so you still want to be able to use arrow functions with it (because it's more convenient and makes no difference to performance as described)

A tslint rule for disallowing arrow function on props only for components that inherit from PureComponent would be an interesting distinguishing feature from eslint, since it could be implemented by leveraging the type system to detect whether a component inherits from PureComponent or not, while in eslint this is not possible.

[adidahiya]

Ok, thanks for the explanation.

your "Button" is not a PureComponent so you still want to be able to use arrow functions with it

this starts to feel unintuitive... so now I'm going to design my components so that only ones with "complex" render logic are PureComponent, while the ones which are thin wrappers inherit from Component instead? where do you draw the line? this rule seems like it would get pretty hard to reason about for the average dev. has anyone used this in practice as a custom rule in their React codebase?

[koba04]

@benny-medflyt Thank you for your great explanation!

I think that applying the rule to PureComponent makes sense. But we should care about not only PureComponent but also Component having shouldComponentUpdate or wrapped by React.memo. It seems to be hard to cover all cases and we can't detect this rule if the Component is wrapped by a Component not optimized so I think it makes sense to ignore DOMComponent.

[adidahiya]

Closing due to inactivity and age. See #210