4

I'm developing a simple 'to do list' react app (new to React.js). I have adding items to a list working but deleting items raises a question. Within my parent react component, i have the following code:

import ToDoEntries from './to_do_entries.jsx';

class ToDoList extends React.Component {
  constructor(props) {
    super(props);
    this.state = { list: [] }
    this.add = this.addItem.bind(this);
    this.removeItem = this.removeItem.bind(this);
  }

  addItem(e) { //removed to avoid tl:dr }

  render() {
    return(
      <form onSubmit={this.add}>
        <input placeholder='Enter item' type='text' ref={(el) => {this._input = el;} }/>
        <button>Add</button>
      </form>

      <ToDoEntries entries={this.state.list}
        removeCallback={this.removeItem}
      />
    );
  }

}

My to_do_entries.jsx component:

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

  renderItems() {
    const { entries, removeCallback } = this.props;

    function createTasks(item) {
      return <li key={item.key}>{item.text}</li>
    }

    var listItems = entries.map(function(item) {
      return(<li onClick={removeCallback} key={item.key}>{item.text}</li>)
    })

    return listItems;
  }

  render() {
    var todoEntries = this.renderItems();

    return(
      <ul>
        {todoEntries}
      </ul>
    );
  }
}

export default ToDoEntries;

Running this code bring:

Warning: setState(…): Cannot update during an existing state transition

Question:

why does to_do_entries.jsx's render immediately execute the callback when an item gets added i.e:

var listItems = entries.map(function(item) {
  return(<li onClick={removeCallback(id)} key={item.key}>{item.text}</li>)
})

However, adding .bind(null, id) to removeCallback ie. <li onClick={removeCallback.bind(null, id)} /> does not?

Mayank Shukla
  • 100,735
  • 18
  • 158
  • 142
soups
  • 53
  • 4
  • So if I understand correctly you are asking why `onClick={removeCallback(id)}` is firing/calling removeCallback immediately and why `onClick={removeCallback.bind(null, id)}` does not fire/call removeCallback immediately? – John Jul 16 '17 at 01:07
  • Exactly yes. Javascript's `this` context and function callbacks i.e `functionName()` vs `function` has been mind-boggling me for some time now. @John – soups Jul 16 '17 at 02:39

3 Answers3

4

Problem is in this part:

onClick={removeCallback(id)}

We need to pass a function to onClick, not the value. When we use () with functionName, that means you are calling that method and assigning the result of that to onClick, that will create a infinite loop if you do setState in removeCallback, because of this cycle:

render ->  removeCallback()  ->  setState ->
  ^                                         |
  |                                         |
  |                                         |
   -----------------------------------------

That's why you are getting the error.

Check the snippet for difference between abc and abc():

function abc(){
   return 'Hello';
}

console.log('without () = ', abc);     //will return the function
 
console.log('with () = ', abc());      //will return the function result (value)

Why it is working with onClick={removeCallback.bind(null, id)}?

Because bind will create a new function, and assign that function to click event, here removeCallback will get called when you click on any item not automatically.

As per MDN Doc:

The bind() function creates a new bound function (BF). A BF is an exotic function object (a term from ECMAScript 2015) that wraps the original function object. Calling a BF generally results in the execution of its wrapped function.

Check the React DOC: Handling events in JSX.

Check this answer for more details about bind: Use of the JavaScript 'bind' method

Mayank Shukla
  • 100,735
  • 18
  • 158
  • 142
  • Thanks @mayank. If 'removeCallback()' calls function and assigns the result, how can i pass an argument back to parent ToDoList component from ToDoEntries? – soups Jul 16 '17 at 02:35
  • 2
    @soups I'm no react expert, but two solutions I see are you could use bind like you are doing now or you could create a wrapper function like this `onClick={()=>removeCallback(id)}` (note that code uses an [arrow function](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions)) – John Jul 16 '17 at 02:54
  • 1
    soups, sorry but just wanted to let you know @mayank did not respond to your question in the comments I did. Mayank seems like a react expert. ;) – John Jul 16 '17 at 04:15
  • 2
    @John i am not a react expert, i slept after answering this one that's why didn't answer, thanks for the comment :) – Mayank Shukla Jul 16 '17 at 04:34
  • @john Apologies for mistakingly tagging mayank on my previous comment. :] – soups Jul 16 '17 at 07:00
  • 1
    @soups No, doing an arrow function instead of .bind is doing the exact same thing. You'll want to avoid this as a solution as it'll create a new function every time it's rendered. – Win Jul 16 '17 at 11:07
2

I would advise against that, and use a similar approach to the example I have written for you. Render a list of todo's that is bind-ed to state and then pass in the relevant information back up to your parent component to remove the item. In this case, I use the index of the todo to splice the array so that it removes it.

Your current onClick is invoked immediately when each todo <li> is rendered because it's simply a function call which is causing the problem. .bind solves this problem because it will create a new function when you click on the element which is why the function doesn't invoke immediately.

However, this is generally considered bad practice because every time the component it'll create this function again and again and again. Multiple this by the amount of todo's on the screen and you'll be losing performance. It's a small issue, but my example shows how to solve this problem. https://codepen.io/w7sang/pen/VWNLJp

// App
class App extends React.Component{
  constructor(props) {
    super(props);
    this.state = { list: [] }
    this.add = this.addItem.bind(this);
    this.removeItem = this.removeItem.bind(this);
  }
  addItem(e) { 
    e.preventDefault();
    this.setState({
      list: [ 
        ...this.state.list, 
        {
          key: Math.random(1,10000),
          text: this._input.value
        }
      ]
    })
  }
  removeItem(payload){
    this.setState({
      list: [ 
        ...this.state.list.slice(0, payload.index),
        ...this.state.list.slice(payload.index + 1)
      ]
    })
  }
  render() {
    return(
      <div>
        <form onSubmit={this.add}>
          <input placeholder='Enter item' type='text' ref={(el) => {this._input = el;} }/>
          <button>Add</button>
        </form>
        <ToDoEntries entries={this.state.list} removeItem={this.removeItem} />
      </div>
    );
  }
}

// TodoEntries [STATELESS]
const ToDoEntries = ({ entries, removeItem } ) => {
  return(
    <ul>
      { entries.map((item, index) => {
        return(<Todo key={item.key} index={index} item={item} removeItem={removeItem} />)
      }) }
    </ul>
  );
}

// Todo
class Todo extends React.Component {
  constructor(props){
    super(props);
    this.state = {};
    this.remove = this.remove.bind(this);
  }
  remove() {
    const { index, removeItem } = this.props;
    removeItem({
      index
    });
  }
  render() {
    return <li onClick={this.remove}>{this.props.item.text}</li>
  }
}

ReactDOM.render(<App />,document.getElementById('app'));
<div id="app"></div>
Win
  • 5,498
  • 2
  • 15
  • 20
  • I was looking for a way around using inline `.bind()` i did come an article explaining why this was a bad idea but i didnt know how to implement it in my example. thanks for this. Just bear with me, but can i assume your solution works due to the reason `const ToDoEntries` does not have a `render()`? @Win – soups Jul 16 '17 at 03:08
  • 1
    @soups Correct. The function will only execute if the element is clicked, you aren't creating a new function every time it is rendered because it is a part of the classes constructor. Creating a new function every time it is rendered is considered heavy. – Win Jul 16 '17 at 11:12
  • i have been pulling my hair over your solution. how is `App#removeItem()` receiving parameter because when `Todo` calls the block, it does so without any argument. In other words, how does `App#removeItem()` know which item to not include in `state: list[]` ? @Win – soups Jul 17 '17 at 20:40
  • 1
    @soups `App#removeItem` receives a payload from a child component which in this case it is `Todo`. When I render the todos in `ToDoEntries`, I also pass over the index. so when the user clicks the element, it also passes the index from the prop in a payload back to the parent component so that it can now remove the todo (with slice) from the state since we know the index of the todo in the array. – Win Jul 18 '17 at 09:59
  • i missed the part where `Todo#remove` executes the callback passing in the parameter. thanks @Win – soups Jul 18 '17 at 21:25
2

why does to_do_entries.jsx's render immediately execute the callback?

Well, when your mapping over the list of todo's each <li/> is invoking the removeCallback function instead of assigning it to the onClick.

So current code

<li onClick={removeCallback(id)} </li>

is equivalent to:

var result = removeCallback(id);
<li onClick={result} </li>

You have correctly pointed out that using bind will work. This is due to the behavior which makes it so useful in these situations.

See the mdn docs for more info, but I'll quote the important part here:

bind ... creates and returns a new function that, when called...

In your case, when using bind, and giving that to the onClick you are creating a new function that will be called when the click event is actually fired, not when the element is being rendered.

Another way of looking at removeCallback.bind(null, id) is its like this:

var newFunc = () => {
  return removeCallback(id);
}
<li onClick={newFunc} </li>
enjoylife
  • 3,801
  • 1
  • 24
  • 33
  • im starting to grasp bind() now, i was struggling with this concept along with `this` context. So as a general footnote, `bind()` creates a new function but as a callback that wont execute immediately. Is this a Javascript thing or react or ES6? – soups Jul 16 '17 at 02:46