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

[alexfedoseev]

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.

[alexfedoseev]

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.

[alexfedoseev]

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

[ljharb]

@benjick using a class component, and binding the method in the constructor. Once class properties become standard, those are a fine alternative as well.

[benjick]

I'm using classes now, could you show me an example?

[ljharb]

class Foo extends React.Component {
  constructor(props) {
    super(props);

    this.foo = this.foo.bind(this);
  }

  foo() {
    doSomething();
    return this.props.blah;
  }

  render() {
    return <button type="button" onClick={this.foo}>blah</button>
  }
}

Once class properties are standardized, you could do:

class Foo extends React.Component {
  foo = this.foo.bind(this);

  foo() {
    doSomething();
    return this.props.blah;
  }

  render() {
    return <button type="button" onClick={this.foo}>blah</button>
  }
}

or

class Foo extends React.Component {
  foo = () => {
    doSomething();
    return this.props.blah;
  }

  render() {
    return <button type="button" onClick={this.foo}>blah</button>
  }
}

(but the last example means the implementation of "foo" isn't shared across all instances, so it's both less performant and less testable)

[benjick]

Thanks, but if I want to do onClick={this.foo(myVar)}?

[ljharb]

@benjick if you have to pass a variable to your function that's potentially different on every render, there's not really anything you can do to retain performance. At that point i'd just make an arrow function inline.

[xyxiao001]

翻译了半天 终于看懂了

[zombieJ]

How about this?

class Foo extends Component {
    constructor(props) {
        ...
        this.cacheTabFunc = {};
        ...
    }

    setTab(name) {
        return (this.cacheTabFunc[name] = this.cacheTabFunc[name] || () => this.setState({tab: name}));
    }
    ...
}

[ljharb]

That's a bunch of boilerplate code with the sole difference that you defer creation of the function until its first use. What's the goal there?

[kodermax]

what is that ?

s/changeField/this.changeField

[ljharb]

It's common dev parlance for "substitute changeField with this.changeField"

[yangmillstheory]

I like the approach of creating a new React Component here, or passing in the local view model into an existing Component that encapsulates the list item, but it increases the prop surface area of the Component.

It's a tradeoff, I guess.

[xgqfrms-GitHub]

bind(this) in constructor & property initializer syntax

class Toggle extends React.Component {
  constructor(props) {
    super(props);
    this.state = {isToggleOn: true};

    // This binding is necessary to make `this` work in the callback
    this.handleClick = this.handleClick.bind(this);
  }

  handleClick() {
    this.setState(prevState => ({
      isToggleOn: !prevState.isToggleOn
    }));
  }

  render() {
    return (
      <button onClick={this.handleClick}>
        {this.state.isToggleOn ? 'ON' : 'OFF'}
      </button>
    );
  }
}

ReactDOM.render(
  <Toggle />,
  document.getElementById('root')
);

class LoggingButton extends React.Component {
  // This syntax ensures `this` is bound within handleClick.
  // Warning: this is *experimental* syntax.
  handleClick = () => {
    console.log('this is:', this);
  }

  render() {
    return (
      <button onClick={this.handleClick}>
        Click me
      </button>
    );
  }
}

https://facebook.github.io/react/docs/handling-events.html

[ljharb]

That works fine. However, you'd also need to do the same thing in componentWillReceiveProps, otherwise the bound functions wouldn't update along with the fields.

[scamden]

just gonna leave this here: https://stackoverflow.com/questions/43968779/are-lambda-in-jsx-attributes-an-anti-pattern/43968902#43968902...

[ljharb]

@scamden Per that link: your component should not know whether any of the children it's rendering are pure components or not, and in fact it should assume all of them are. Separately, it's wrong; they have a measured, albeit small, impact on rendering performance - and a much larger impact on class performance in JS in general, far outside of React.

It's also not just about performance; using a constructor-bound instance method is a much more ideologically correct approach.

[scamden]

I think it's the "albeit small" part that's not making it into most of the
discussions i'm reading on this topic. There are always performance
optimizations to make but especially when they're small it's not always
worth the trade off. My point here is that performance is put forward as
the primary driver for this rule when the real world perf impact is likely
negligible. I would at least love to see some good comparative profiles if
perf is going to be tossed around as the reason.

As for ideology and style, I find numerous examples where an inline lambda
is a much more elegant pattern and is much less verbose. Function currying
being a good example. I still have yet to see a single solution to
rendering a component that requires a callback in a loop without giving up
the Stateless nature of that component. I would be very interested if you
are able to give a good solution to that. (The only one given in this
thread points to the eslint example that uses a class component where the
callback references this.props). In terms of elegance, it doesn't seem
great to require loop components (which are very likely presentational) not
to be Stateless functions.

On Tue, Jun 6, 2017 at 5:14 PM Jordan Harband [e-mail]
wrote:

@scamden https://github.com/scamden Per that link: your component
should not know whether any of the children it's rendering are pure
components or not, and in fact it should assume all of them are.
Separately, it's wrong; they have a measured, albeit small, impact on
rendering performance - and a much larger impact on class performance
in JS in general, far outside of React.

It's also not just about performance; using a constructor-bound instance
method is a much more ideologically correct approach.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/airbnb/javascript/issues/659#issuecomment-306649510,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABE5mAbIvzseUq54A6G7aTw7PgCOL9Glks5sBetjgaJpZM4G_rTw
.

[scamden]

To be clear the loop problem I was referring to is this:

{
myThings.map((myThing) => {
  return <NiceElegantStatelessLinkComponent onClick={() => doSomething(myThing)} />;
});
}

const NiceElegantStatelessLinkComponent: StatelessComponent<{ onClick: () => void }> =
 (props) => <a onClick={props.onClick}>some text </a>;

the solution given was to make the StatelessComponent a class

{
myThings.map((myThing) => {
  return <NotSoElegantLinkComponent onClick={doSomething} myThing={myThing} />;
});
}

class NotSoElegantLinkComponent extends Component<{ myThing: MyThingType, onClick: (myThing: MyThingType) => void }, void> {

  constructor() {
    super();
    this.onClick = this.onClick.bind(this);
  }

  onClick() {
    this.props.onClick(this.props.myThing);
  }

  render() {
    return <a onClick={this.onClick}>some text </a>;
  }
}

Would love to know if there's any solution to not using lambdas here that doesn't require my going against the Stateless / Functional pattern

[ljharb]

Not that I know of - a bound function, or one that retains a closure to this (like an arrow function), is state. Thus, it requires a stateful component.

[scamden]

I don't think it's fair to call a bound function state. It's not anymore "state" than the value it encapsulates (which would otherwise be a prop or in functional terms an argument). It's certainly not state not in the sense that React defines "Stateless". I wasn't implying that the class solution makes the component stateful. It still only uses props but the syntax is much less elegant and the fact that it is not defined as a pure function opens the door to state and makes testing more difficult. Just seems like lambdas are a fundamental pattern especially in functional models. Point being: It doesn't seem obvious that constructor bound methods are more elegant.

[ljharb]

(Just a quick note; arrow functions aren't really "lambdas"; that's not the right term to be using for JS)

[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 [e-mail]
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
https://github.com/airbnb/javascript/issues/659#issuecomment-338706832,
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 [e-mail]
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
https://github.com/airbnb/javascript/issues/659#issuecomment-338739176,
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.

https://github.com/yannickcr/eslint-plugin-react/pull/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 [e-mail]
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
https://github.com/airbnb/javascript/issues/659#issuecomment-338753277,
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 [e-mail]
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
https://github.com/airbnb/javascript/issues/659#issuecomment-338760472,
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.