react/jsx-no-bind: how to pass args from callbacks?

[alex35mil]

I have a question about react/jsx-no-bind rule in 3.0.0: how can I pass args to handler from callback if I can't use bind and arrow functions?

doSomething(thing) { ... }

render() {
  const { items } = this.props;

  return (
    <ul>
    {
      items.map(item => (
        <li onClick={this.doSomething.bind(this, item)} /> // <--- JSX props should not use .bind()
        <li onClick={() => this.doSomething(item)} />      // <--- JSX props should not use arrow functions
      ))
    }
    </ul>
  );
}

[ljharb]

With this rule, you'd not be able to do either. In this case, however, I'd think you'd want to use a single onClick on the <ul> though, and use the event's target (ie, event delegation) so that you have 1 handler instead of N - which also neatly bypasses this issue.

I suspect that in all cases where this rule seems problematic, there will be a more ideal approach that obviates the problem.

[alex35mil]

So in most of the cases suggestion is to save the data to the DOM and fetch it from there via e.target, is this correct?

[ljharb]

Ah, that's a fair point. you may not want to do that either. You could use the index of the <li> to look it up in this.props.items, but that seems a bit janky.

Can you be a bit more specific about your use case? I'm curious what prompts this to come up.

[geekjuice]

There are some tips in the eslint-plugin-react#jsx-no-bind documentation.

tl;dr, you can create another React component for the list items where you pass in the item and the click handler and pass the item as a param.

[ljharb]

@geekjuice thanks, that's very helpful!

[geekjuice]

If you still want to do it inline, it's possible to use something like to _.partial or

function partial(fn, ...args) {
  return fn.bind(fn, ...args);
}

React.createClass({
  handler(item) {
    // do something
  },

  render() {
    const {items} = this.props;
    return items.map(item => {
      return <div onClick={partial(this.handler, item)} />;
    });
  }
});

This will pass through the linter, but will still have the issues listed in eslint-plugin-react#jsx-no-bind documentation.

[ljharb]

Right - at that point you might as well disable the rule.

[alex35mil]

I guess another React component is the best solution with this rule enabled. Thanks, guys, closing this.

[Kerumen]

Just came up with this issue for the "new" way of passing ref. Instead of a string you should now use arrow functions: https://facebook.github.io/react/docs/more-about-refs.html

A recent commit in the plugin introduces flexibility with the rule and allow arrow functions in ref attributes.

Can you add this?

[ljharb]

That link says "Also note that when writing refs with inline function expressions as in the examples here, React sees a different function object each time so on every update, ref will be called with null immediately before it's called with the component instance." - it still seems to me that it's an antipattern to create functions in the render path.

[Kerumen]

Okey then how should I create refs? Like this?

mapRef(c) {
  this._input = c;
}

render() {
  <input ref={::this.mapRef} />
}

[ljharb]

:: is just the same as this.mapRef.bind(this) so no, you'd do this.mapRef = this.mapRef.bind(this) in the constructor, and then just ref={this.mapRef} in render.

[Kerumen]

I wanted to keep it short and avoid the constructor declaration.
Does this affect performance to create functions in render or it's only for design pattern reasons?

[ljharb]

If you're using this then you can't/shouldn't avoid the constructor.

It affects performance significantly, because react's DOM diffing can't use === to compare prop values when you create a new function on every render.

[Kerumen]

Okey I understand. And what about es7 class properties ?

mapRef = (c) => {
  this._input = c;
};

It'll bind this without the constructor.

[ljharb]

Although those aren't standard yet, those will likely work as if they were run in the constructor - so that would work just fine!

[Kerumen]

Okey! Thanks for your help, gladly appreciated! :)

[alexprice1]

What do you think of this approach? Each function is created once in the constructor and that is it.

class Form extends React.Component {
  constructor() {
    const fields = this.props.fields;

    this.changeField = {};
    this.fields.forEach((field) => {
      this.changeField[field] = this.onChange.bind(this, field.name); 
    });
  }

  onChange(fieldName, event) {
    this.props.onChange(fieldName, event.target.value);
  }

  render() {
    const fields = this.props.fields.map((field) => {
      return <input value={field.value} onChange={this.changeField[field.name]}/>
    });

    return (
      <div>
        {fields}
      </div>
    );
  }
}

[ljharb]

s/changeField/this.changeField inside the forEach and sure, that works great!

[alexprice1]

Cool, thanks!

[Dindaleon]

Hello there, sorry if my question is a bit off topic but I would like to take the opportunity to ask whether I need a constructor or not for declaring the state if I am using es6 and es7.

Currently, in my code, i doing this:

state = { ... }

someFunction = () => { ... }

Instead of:

constructor(props) {
  super(props)
  this.state = { ... }
}

What would the difference be? Do I always need the constructor?

[ljharb]

@Dindaleon the former is the "class properties" proposal, which is not yet standard (still stage 1 or 2, I think). The latter is the only way to do it currently.

[supnate]

I created a simple utility memobind to address this problem, any comment is welcome. https://github.com/supnate/memobind

[benjick]

What about something like this?

foo = bar => () => {
  this.props.myFunc(bar);
}

[ljharb]

@benjick then this.foo() would return a new arrow function on every call, which is no better than just making the arrow function inline (and arguably worse, since it's both less clear, and also incurs the overhead of one extra function call)

[alexprice1]

Not true. You would not be recreating foo each time it renders (that's the issue).

[ljharb]

@alexprice91 ah, i see what you mean. How is your example better than just foo = () => { … } and using this.foo in the render?

[benjick]

So what's the real solution do this problem? I need to pass variables

[scamden]

interesting, i would be curious to know what that distinction is. i was actually just using the term because it was in the related linting rule (jsx-no-lambda). but i've always essentially thought of them as the same, what makes them not lambas?

[scamden]

a quick look on wikipedia makes them sound the same:

In several programming languages, anonymous functions are introduced using 
the keyword lambda, and anonymous functions are 
often referred to as lambdas or lambda abstractions.

but i am genuinely curious to learn it, if there is a distinction

[ljharb]

I guess I'd never really thought about it; https://gist.github.com/ericelliott/414be9be82128443f6df has interesting discussion. In general to me it evokes lambda calculus meanings, but I suppose it's fine describing an arrow function ¯\_(ツ)_/¯

As to the topic at hand; it's a fair counterpoint that bindings aren't state. However, in React, because any custom components you render might be pure, the rule I'd probably follow is "whenever you pass a function as a prop on a custom component, be sure it's the same (===) across renders when possible".

[scamden]

That's fair. It def seems like a good practice to avoid creating anonymous functions unnecessarily. just seemed extreme to straight up ban it, when there are some really nice use cases for it. But i suppose you can always ignore or extract them out of jsx as you said above. thanks for the discussion.

[skosch]

I wonder whether the canonical solution – to create a new component with item passed as a prop – is actually any better, performance-wise. Sure: use arrow functions, and they'll be re-bound on every render. But create a new component, and you'll have the overhead of calling extra components and diffing their item props on every re-render as well. So I wouldn't be surprised if in practice, the occasional

items.map(item => (
  <li onClick={() => this.doSomething(item)} />      // <--- JSX props should not use arrow functions
))

isn't actually as bad as it's made out to be. Plus, it makes for such beautifully readable code.

Are there benchmarks on this anywhere?

[ljharb]

It allows react to do the diffing, which it’s capable of doing - if you create a new function, that must always be !== from the previous render.

Separately, in your example, 1) a constructor-bound instance method solves it without creating a new component, and 2) the perf recommendation actually doesn’t apply to functions passed to DOM elements, but the rule doesn’t yet differentiate.

[scamden]

React only does the diffing on props if you are passing props to a PureComponent and I think the point Sebastian is making is that cost of diffing in using a pure component can sometimes outweigh the perf impact of rerendering since react already diffs the output of rendered components. Here is a very good discussion with some actual benchmarks : https://cdb.reacttraining.com/react-inline-functions-and-performance-bdff784f5578

…

On Mon, 23 Oct 2017 at 08:58 Jordan Harband ***@***.***> wrote: It allows *react* to do the diffing, which it’s capable of doing - if you create a new function, that must *always* be !== from the previous render. Separately, in your example, 1) a constructor-bound instance method solves it without creating a new component, and 2) the perf recommendation actually doesn’t apply to functions passed to DOM elements, but the rule doesn’t yet differentiate. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#659 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABE5mDm0VovULwA0xrw2NRg3AMkAKtcJks5svLeWgaJpZM4G_rTw> .

[skosch]

@scamden thanks a ton for that link. It articulates my hunch so much better than I could hope to.

I think I'll be turning this eslint rule off – it has taught me not to abuse binds and arrow functions, which I'm grateful for, but beyond that all the warnings are just too much noise. If re-rendering performance ever becomes an issue I'll have to run benchmarks anyway to be sure.

[scamden]

Yep if it becomes an issue you should be able to see it in a profiler! Don’t preoptimize! jsx-no-lambda : false for me

…

On Mon, Oct 23, 2017 at 10:42 AM Sebastian Kosch ***@***.***> wrote: @scamden <https://github.com/scamden> thanks a ton for that link. It articulates my hunch so much better than I could hope to. I think I'll be turning this eslint rule off – it has taught me not to abuse binds and arrow functions, which I'm grateful for, but beyond that all the warnings are just too much noise. If re-rendering performance ever becomes an issue I'll have to run benchmarks anyway to be sure. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#659 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABE5mIUkzGU2d-ThJFZ3Cw87KsVzgiLnks5svM_tgaJpZM4G_rTw> .

[ljharb]

It's been an issue on airbnb.com many times; and it's often very hard to debug where these issues are occurring.

I do agree that the jsx-no-bind rule (again, the term "lambda" isn't actually appropriate anywhere) is overzealous; it shouldn't warn on arrow functions used on HTML elements, only on custom components, and only in the render path.

jsx-eslint/eslint-plugin-react#1459 will reduce the noise of this rule soon; I'd recommend not turning it off.

[skosch]

Ahh ok @ljharb that PR would indeed solve most (maybe all) of my issues.

[scamden]

Jordan did you have profiler data to share for those cases where it has been an issue? I’d be curious to know the circumstances that create issues because as the author mentions in the link above, outcomes can turn out worse by moving arrow functions to the constructor depending how frequently props change and how frequently components are created. Def doesn’t seem clear that this is a 100% no brainer win even in cases that are pure components and not elements. Which is why my approach would be not to pre-optimize. Profiling can be tricky for sure but aren’t we forced to profile if performance issues can arise both with or without the application of this rule? That’s the unfortunate thing about performance management is that it takes hard work and measurement, and catch all rules can often have effects opposite to our expectations if we aren’t measuring.

…

On Mon, Oct 23, 2017 at 11:29 AM Sebastian Kosch ***@***.***> wrote: Ahh ok @ljharb <https://github.com/ljharb> that PR would indeed solve most (maybe all) of my issues. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#659 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABE5mOSSPHraniz8XzYWYFFkVrZcP-8Tks5svNsUgaJpZM4G_rTw> .

[ljharb]

No, I don't have that data available; but also the profiling data was useless in locating the issue. We instead found places where this rule was overridden, and fixed them, and things got faster.

Functions can only be compared with ===; PureComponents are not allowed to skip a render if any of their function props aren't strictly equal. It's 100% a no-brainer win here.

[scamden]

You're missing the point that it's not always faster to use a pure component. The fact that it has to do the diff at all can make it slower if the props change frequently. I would recommend reading the article I posted if you didn't already. It really does make some good points. There's a reason the Knuth quote has become a cliche: "Premature optimization is the root of all evil". It's great that you were able to make your UI faster be removing lambdas (which I assume you know not by subjective observation but by looking at profiles). I was never suggesting that you have to use profiles in order to try to locate the problem (although sometimes that's the only way if you don't already have a theory). My suggestion is that you use them to verify performance theories like you did in this case. That aside, the fact that it made it faster in the cases you tried is not sufficient evidence that it will always make it faster and that is basically my whole point: it's is extremely rare to find a rule that always makes things faster and that's why we always have to measure and only optimize when we see a problem. If you read the article you will see cases where following this rule actually makes rendering slower. My basic point is not that It isn't a good idea to avoid lambdas in cases where you're using a pure component and you have measured that case to ensure that using a pure component is actually faster. My point is that it's not a good idea to make any performance optimization a rule, but rather we should think of them as possible fixes when we are measuring and optimizing. Anyway, luckily the beauty of tslint is that we can opt out, so I don't mean to beat a dead horse, but I appreciate you're willingness to discuss.

…

On Mon, 23 Oct 2017 at 11:54 Jordan Harband ***@***.***> wrote: No, I don't have that data available; but also the profiling data was useless in locating the issue. We instead found places where this rule was overridden, and fixed them, and things got faster. Functions can only be compared with ===; PureComponents *are not allowed to skip a render* if any of their function props aren't strictly equal. It's 100% a no-brainer win here. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#659 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABE5mCWnuhfiHlxdyrbGv4uvmg3vNOXcks5svODjgaJpZM4G_rTw> .

[duleep-noetic]

you could do something like this.

class Example extends React.PureComponent {
    constructor(props) {
        super(props);
        this.handleClick = this.handleClick.bind(this);
    }

    handleClick(id) {
        return (event) => this.props.doSomething(id);
    }

    render() {
        <ul>
            {this.props.listOfSomeKind.map(thing =>
                <li onClick={this.handleClick(thing.id)}>
                    {thing.name}
                </li>
            )}
        </ul>
    }

Use bind inside the constructor rather than inside the render so that it doesn't have to redefine functions every time when it renders.

[ljharb]

Absolutely do not do that - you’re calling handleClick, which doesn’t exist, and you don’t want to be creating a new function in every render.

[duleep-noetic]

@ljharb im binding the callback in the constructor.constructor is only called once, not on every render..i think this is the recommended way in react docs. https://reactjs.org/docs/faq-functions.html#how-do-i-bind-a-function-to-a-component-instance.(es2015). and hence this.handleClick exist. if you define bind inside render method then it will be creating a new function every time it renders.

[ljharb]

@duleep-noetic i agree with what you’re saying, but your code example has onClick={this.handleClick(blah)} which invokes the function every single render, and passes its return value as a prop.

[duleep-noetic]

@ljharb i think calling the function instead of passing the function to onClick will invoke it in every render..in JS, funcName(...) is always an immediate function-call. i.e. onClick: handleClick() sets onClick to the return value of handleClick(). but whereas in bind the return value is a new function, when called boundFunction() actually calls handleClick().Is it or did i miss something?

[ljharb]

@duleep-noetic yes, i'm saying that is the problem. onClick should be a reference to a function, and should not change across renders.

The proper way to do what you're suggesting is something more like this:

class Example extends React.PureComponent {
    constructor(props) {
        super(props);
        this.handleListClick = this.handleListClick.bind(this);
        this.createThingOnClicks(this.props.listOfSomeKind);
    }

    componentWillReceiveProps(nextProps) {
      this.createThingOnClicks(nextProps.listOfSomeKind);
    }

    createThingOnClicks(things) {
      this.thingClicks = things.reduce((acc, thing) => ({
        ...acc,
        [thing.id]: (event) => { /* do something with `thing` */ },
      }), {});
    }

    handleListClick(e) {
      // do something
    }

    render() {
      return (
        <ul onClick={this.handleListClick}>
            {this.props.listOfSomeKind.map(thing => (
                <li onClick={this.thingClicks[thing.id]}>
                    {thing.name}
                </li>
            ))}
        </ul>
      );
    }
}

[geoguide]

Okay, I understand conceptually, breaking the item out into it's own component, but if I have a pagination component and I break Link out into it's own component, it's still going to create a function for each Link anyway, am I really saving anything?

[ljharb]

@geoguide yes, it would save on rerenders of Link, potentially.

[ElvenMonky]

switching from functional components to class components just for event handlers sounds a bit silly

[ljharb]

@ElvenMonky why? They’re stateless functional components, and event handlers are state.

[ElvenMonky]

Event handlers have nothing to do with state. They are just functions.

Another note that jsx-no-bind doesn't make much sense for bindings to events in Rect Elements, because they go to Synthetic Event System, which doesn't update DOM when handler instance changes.

[ljharb]

@ElvenMonky to the component that receives them they are indeed props; but to the component that creates them, they are state.

I agree that the rule doesn't make much sense when applied to event handlers to DOM elements only.

[ElvenMonky]

Sorry, for editing my comment. My point is that event handlers are simple functions that can come from everywhere (defined in module scope, or on/in function itself, be imported from other modules). They are not necessarily come from state, neither they have to deal with state in any way (like calling setState).

More over state is specific property of each component, that almost never includes any functions, unless they can change dynamically,

[ljharb]

They are conceptually state, whether they come from component state or not. Instance properties are also state.