added documentation for passing arguments to event handlers

[swyxio]

hi, addressing issue #75 !

[reactjs-bot]

Deploy preview ready!

Built with commit 7f8d084

https://deploy-preview-81--reactjs.netlify.com

[bvaughn]

Please remove package-lock.json from this PR 😄

[swyxio]

sorry bout that. package-lock never used to matter when i was just working on my little repos. removed and recommitted.

[bvaughn]

This line isn't actually true ^

function foo() {}
const ctx = {};
const a = foo.bind(ctx);
const b = foo.bind(ctx);
console.log(a == b); // false

Binding in the constructor is a way to avoid unnecessary updates but that's not an option when you want to pass in-scope variables like this.

That being said, I think we should still show the .bind() form for people who prefer to write vanilla ES5 but it doesn't perform differently in terms of shouldComponentUpdate (at least not that I'm aware of).

One way to avoid unnecessary renders would be:

class Parent extends Component {
  // This property will be initialized once during instantiation
  // Subsequent renders will use the same function
  deleteRow = (index) => {};

  render() {
    // Render stuff, including:
    <DeleteRowButton index={i} deleteRow={this.deleteRow} />
  }
}

class DeleteRowButton extends Component {
  // This property will be initialized once during instantiation
  // Subsequent renders will use the same function
  onClick = () => {
    this.props.deleteRow(this.props.index);
  };

  render() {
    return <button onClick={this.onClick}>Delete Row</button>;
  }
}

But all of this ^ is overkill for a simple <button>. It would only make sense to do if the component you're rendering is expensive (eg has as lot of nested children or complex logic).

[swyxio]

i think the examples conflate the method of passing params to events (where this pattern is valid) with passing params within loops (where this pattern is not valid as you point out). I should be clearer there AND also provide the hint to pass by props as newbies can overlook that simple solution. I agree the rerendering thing is overkill and if the reader is worrying about perf to that extent they probably wouldnt be reading this section of the docs anyway. Was only including it because it was suggested in the issue raised.

[alexkrolick]

I added some comments about how and when to avoid binds to the FAQ page (#43). It's probably OK to keep it high-level here.

[bvaughn]

It's probably OK to keep it high-level here.

Agreed.

[swyxio]

Parsimony is a virtue when it comes to docs. I will just delete the problematic line of code so as not to mislead/confuse.

[devuxer]

I understand your thinking, but I actually disagree with the decision you guys ended up making. I came to this thread specifically because I was confused why there is a warning about re-renders for onClick={(e) => this.handleClick(e)} but not for onClick={(e) => this.deleteRow(id, e)}.

Also, being that I'm a relative newbie to React, I didn't fully understand the ramifications of these potential re-renders. The discussion here actually helped me get a sense of how serious a problem they might be from a performance standpoint.

I understand not wanting to pollute the documentation with too much detail, but it would be nice to put the extra details in something akin to footnotes.

[swyxio]

Pushed and awaiting your review. sorry for the original problematic line (i'm still not super comfortable when it comes to questions of context)

[bvaughn]

Thanks a bunch for the changes 😄

[gaearon]

Tweaked in db1262d.

"Param" is not explained, and people might confuse it with some React term. "Parameter" is more explicit.

Also added reference links.

Finally, these two lines weren't equivalent, and people would likely ask how to get access to the event. So I made them consistent.

Thanks for working on this change!

[bvaughn]

Nice changes Dan. Thanks for adding to the section.