Skip to content
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

Allow exceptions to "jsx-no-lambda" rule #26

Closed
tp opened this issue Jul 12, 2016 · 10 comments
Closed

Allow exceptions to "jsx-no-lambda" rule #26

tp opened this issue Jul 12, 2016 · 10 comments

Comments

@tp
Copy link

tp commented Jul 12, 2016

I think the usage of lambdas in ref attributes can warrant an exception from the "jsx-no-lambda" rules, since this is an easy way to get type-safe access to rendered elements.

The only alternative I see would be to create instance functions which would be cumbersome to write and still have to be bound using .bind(this).

So until we have a nicer way to generate these "bindings" for example using macros, it would be nice to allow lambdas on some attributes.

@adidahiya
Copy link
Contributor

Doesn't that result in the same issues as any other lambda in a JSX prop? Personally I like using a pattern like this for ref handlers:

class Button extends React.Component<{}, {}> {
    private buttonElement: HTMLElement;
    private refHandlers = {
        button: (ref) => this.buttonElement = ref,
        ...
    };

    public render() {
        return <button ref={this.refHandlers.button} />;
    }
}

@jkillian any thoughts on a whitelist of exceptions for this rule?

@jkillian
Copy link
Contributor

A whitelist seems like unnecessary extra complexity to me. If a lamda is needed, it can be declared and initialized before the JSX code.

As far as the case of ref, there's no need to recreate the lambda on every render, which is exactly what this rule is trying to prevent. Something like the code example above (the object literal isn't necessary) works well.

@jkillian
Copy link
Contributor

jkillian commented Oct 5, 2016

Closing, as I don't think we'll take any action here

@jkillian jkillian closed this as completed Oct 5, 2016
@testerez
Copy link
Contributor

testerez commented Jul 7, 2017

@jkillian in react the performance drop with arrow function as prop is due to pure components re-rendering when it could be avoided. The creation on a new function is not a problem.

The ref attribute is not part of the props so it's not a problem to use an arrow function here.

See: facebook/react#9086 (comment)

All React examples I could find recommend the usage of an arrow function in ref and alternatives seem needlessly complicated.

IMO the fact that this rule warns on ref makes it pretty useless.

@adidahiya
Copy link
Contributor

The ref attribute is not part of the props so it's not a problem to use an arrow function here.

This is a good point. I would accept a PR to whitelist certain props like ref.

@bigshahan
Copy link

bigshahan commented Oct 23, 2017

ref is not part of the props, but would like the ability to keep the linter error for ref attribute, as when it is provided as a lambda, the ref gets called to unset it and then again to set it again, on every render. Should I create a new PR to bring in as an option?

Source: https://reactjs.org/docs/refs-and-the-dom.html#caveats

@testerez
Copy link
Contributor

@shahan312 did you mesure any actual performance loss related to that?

@mitchlloyd
Copy link

@adidahiya wrote:

I would accept a PR to whitelist certain props like ref.

However #107, just permitted "ref" and did not introduce a whitelist.

Hard to say how significant the performance impact is that @shahan312 is raising (I'm generally onboard with @testerez's line of thinking), but it seems as reasonable a concern as any I've seen on this topic. My own interest in a whitelist is to allow anonymous functions for render props which are often used with closure scoping (e.g. <Route /> from react-router).

With this in mind, I've opened a PR to introduce a configurable whitelist. Please let me know what you think!

@msaspence
Copy link

msaspence commented Dec 9, 2017

@adidahiya how about the ability to allow on DOM components where he performance implications would be limited?

Essentially this suggestion for eslint jsx-eslint/eslint-plugin-react#1238

@adidahiya
Copy link
Contributor

Closing due to deprecation, see #210

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants