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

Binding in react #7385

Closed
sandysaders opened this issue Jul 31, 2016 · 22 comments
Closed

Binding in react #7385

sandysaders opened this issue Jul 31, 2016 · 22 comments

Comments

@sandysaders
Copy link

sandysaders commented Jul 31, 2016

I recently read an article on Medium about binding in react. Basically explaining how you should bind functions inside of the constructor of stateful components to prevent unnecessary rerenders in a component. It shows how using arrow function and .bind causes function creation on the spot and makes it seem like new props are being received continuously even when it may not. This is bad and prevents optimizations according to the article. So great I know I should bind in constructor but what about binding methods with parameters not just calling them in stateful and stateless components?

Say theres something like the following:

// Using stateful components
  class Something extends React.Component {
    constructor () {
      super()
      this.doSomething = this.doSomething.bind(this)  
    }
    doSomething (take, take2) {
       return take + take2;
    }

    render () {
      return (
        <div>
          <button onClick={this.doSomething(5, 6)}>clickMe!</button>
        </div>
      );
    }
  }

// Using stateless components
  function Something (props) {

    function doSomething (take, take2) {
       return take + take2;
    }

    return (
      <div>
       <button onClick={doSomething(5, 6)}>clickMe!</button>
      </div>
    );
  }

It throws an error because the call evaluates immediately to a number when it should be a function. This makes sense and the only way around this I've been able to go around this is using an arrow function in the onClick () => doSomething(5, 6) and that will run. But this will cause unnecessary rerenders that the article explained. So how could you do this better and prevent unnecessary rerenders in stateless and stateful components?

@brigand
Copy link
Contributor

brigand commented Jul 31, 2016

This bug tracker is for issues with react itself. For usage questions, please ask on StackOverflow or IRC.

The only solution that comes to mind is some kind of memoization, but there are a lot of ways to do it with different trade offs. In some cases the memoization could be more expensive than the child rerendering.

@hnordt
Copy link
Contributor

hnordt commented Aug 1, 2016

doSomething() could return a function that does the math using the props passed. We transformed doSomething() into a high order function.

I'm not use if it'll prevent unnecessary rerenders. Can you test and send me feedback?

// Using stateful components
  class Something extends React.Component {
    constructor () {
      super()
      this.doSomething = this.doSomething.bind(this)  
    }
    doSomething (take, take2) {
       return event => {
         event.preventDefault()
         return take + take2
       };
    }

    render () {
      return (
        <div>
          <button onClick={this.doSomething(5, 6)}>clickMe!</button>
        </div>
      );
    }
  }

// Define this outside Something()
function doSomething (take, take2) {
       return event => {
         event.preventDefault()
         return take + take2
       };
}

// Using stateless components
  function Something (props) {
    return (
      <div>
       <button onClick={doSomething(5, 6)}>clickMe!</button>
      </div>
    );
  }

@aweary
Copy link
Contributor

aweary commented Aug 1, 2016

Yeah, the issue is that you're invoking your method, not passing it down. You can also pass arguments to bind so that when its invoked it will be invoked with those arguments

this.doSomething = this.doSomething.bind(this, 5, 6)

If you have access to the arguments for doSomething in the initial props you can bind it like that in the constructor and just do

<button onClick={this.doSomething}>clickMe!</button>

I'm not use if it'll prevent unnecessary rerenders. Can you test and send me feedback?

@hnordt your method won't prevent unnecessary renders because you're still returning a new function very time the component is rendered.

@milesj
Copy link
Contributor

milesj commented Aug 1, 2016

Or just use arrow functions.

<button onClick={() => this.doSomething(5, 6)}>clickMe!</button>
<button onClick={() => doSomething(5, 6)}>clickMe!</button>

@zpao zpao closed this as completed Aug 1, 2016
@aweary
Copy link
Contributor

aweary commented Aug 1, 2016

@milesj that would still mean that the onClick function is recreated on every render call

@kennetpostigo
Copy link

@milesj yea using arrow functions cause it to recreate the onClick on every render call @milesj

@gaearon
Copy link
Collaborator

gaearon commented Aug 1, 2016

Re-creating functions in render() is not that expensive.
Please don’t blindly follow advice you see in articles.

It’s not always applicable, and you should measure and test if it makes a difference.

Using arrows (or even bind in render()) only prevents PureRenderMixin-like optimizations which aren’t enabled by default anyway.

So if you don’t use PureRenderMixin, PureComponent, or shallowCompare addon in the components below the tree, you don’t gain anything by binding in constructor.

@gaearon
Copy link
Collaborator

gaearon commented Aug 1, 2016

If you really know that binding was expensive in your case after measuring the performance before and after in your app, but you want to pass some arguments, you can just pass them as props: doSomething={this.doSomething} x={5} y={6}, and let the child component do something like this.props.doSomething(this.props.x, this.props.y).

The benefits of this are dubious if x and y change often because if the data changes, shallow comparisons won’t help you anyway. So this is only useful in rare cases when the code is perf critical and those arguments are relatively stable (e.g. IDs).

@sandysaders
Copy link
Author

sandysaders commented Aug 1, 2016

@gaearon I ask not only because I think its a performance hit to my app at all. I ask because if there is a "better" way or more efficient way of doing things because I am using PureRenderMixin I would rather do it the "best" way now rather than going back later and changing a whole lot of code. It's no pain to me to do this now. I just wasn't sure how to do this without arrow functions or .bind() in the article and couldn't find it in the docs and I was curious really in an onClick handler as this is where I usually use something like this.

It's kind of like the issue going around on twitter where you have developers passing all the props using rest. rather than passing down whats needed. Both work just one is preferred so that it is explicit what is needed. I view it like this. I rather do it right the first time around not because the other way is wrong but because I think it might be beneficial in the slightest way.

@sandysaders
Copy link
Author

@timeche I thought the same. As the child component would need to call the function with the props in a handler as well?

@timche
Copy link

timche commented Aug 1, 2016

@gaearon Why not use the props directly in the this.doSomething function?

Example:

doSomething() {
  return this.props.x + this.props.y
}

render() {
  return <Comp doSomething={this.doSomething} />
}

@gaearon
Copy link
Collaborator

gaearon commented Aug 1, 2016

@timche I think presumably they would be different for each item in a list (that’s the usual use case) but I’m not sure because the example is too trivial.

@gaearon
Copy link
Collaborator

gaearon commented Aug 1, 2016

@sandysaders

I ask because if there is a "better" way or more efficient way of doing things because I am using PureRenderMixin I would rather do it the "best" way now rather than going back later and changing a whole lot of code.

I think the "best" way is to write the code as naturally as you can, and if you have issues, you can optimize individual components. It won't be changing "a whole lot of code", most likely you'll need to tweak a few components for desired perf. Also in most apps you won't even have those issues to begin with.

The funny thing with perf is there is no hard rule that you could just follow every time. We don't recommend making every single component use PureRenderMixin or equivalent. We recommend that you write your app normally, and if you need to optimize it, then you can use React Perf tools to figure out the best components to optimize, and optimize just them.

@timche
Copy link

timche commented Aug 1, 2016

It's no pain to me to do this now. I just wasn't sure how to do this without arrow functions or .bind() in the article and couldn't find it in the docs

@sandysaders take a look here ;)

We recommend that you bind your event handlers in the constructor so they are only bound once for every instance:

@gaearon
Copy link
Collaborator

gaearon commented Aug 1, 2016

And no, you can’t do this with stateless functional components.

So if you insist on never passing arrows down, you either won’t be able to pass callbacks down from stateless functional components.

@sandysaders
Copy link
Author

@gaearon I totally hear you dude. Really was just curious because I had never thought about it how to "properly" bind functions/methods for handlers before.

@timche nice

@timche
Copy link

timche commented Aug 1, 2016

@sandysaders Btw, you can also use arrow functions in your classes with babel-transform-class-properties, so you don't have to bind them anymore.

Example:

class Something extends React.Component {
  doSomething = (x, y) => x + y

  render() {
    return <Comp doSomething={this.doSomething} />
  }
}

@cvazac
Copy link

cvazac commented Aug 2, 2016

We have been using html data attributes when every item of a list needs to know who it is.

render () {
  return (
    <div>
      this.props.items.map((item, index) => {
        return <button data-index={index} onClick={this.onClick} />
      })
    </div>
  )
}
onClick = ({target: {dataset: {index}}}) => {
  alert(this.props.items[index])
}

@gaearon
Copy link
Collaborator

gaearon commented Aug 2, 2016

Reading from DOM is generally not very great if you already have this data provided by React.

@cvazac
Copy link

cvazac commented Aug 2, 2016

yep, i agree. the dataset pattern is just a workaround to avoid writing a new component that just renders the button

render () {
  return (
    <div>
      this.props.items.map((item, index) => {
        return <Button item={item} onClick={this.onClick} />
      })
    </div>
  )
}
onClick = (item) => {
  alert(item)
}

// Button.jsx
render () {
  return <button onClick={this.onClick} />
}
onClick = (item) => {
  const {item, onClick} = this.props
  onClick(item)
}

@ErythroME
Copy link

This article provided an way to pass arguments without bind. Perhaps it helps.

@milesj
Copy link
Contributor

milesj commented Aug 3, 2016

@ErythroME I'd take that article with a grain of salt. The only truth is #2, in that bind() itself can be really slow depending on the browser and environment.

Arrow functions are not inefficient -- this is exactly what their use case is for. If anything, it's the transpilation down to regular functions via babel that is the performance loss.

Furthermore, don't use #4 or #6 as they're not finalized specs or are dead/denied specs.

The best, easiest, and most idiomatic approach in my opinion is #5 (or the 2nd bonus). These should be the standard as it solves most, if not all of use cases.

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

No branches or pull requests