1

I want to call a method inside another method like this, but it is never called.

Button:

<span onClick={this.firstMethod()}>Click me</span>

Component:

class App extends Component {
  constructor(props) {
    super(props);
    this.state = {..};
  }

  firstMethod = () => (event) => {
    console.log("button clicked")
    this.secondMethod();
  }

  secondMethod = () => (event) => {
    console.log("this is never called!")
  }

  render() {..}
}

The first method is called, but not the second one. I tried to add to the constructor

this.secondMethod = this.secondMethod.bind(this);

which is recommended in all the other solutions but nothing seems to work for me. How can I call the second method correctly?

user2212461
  • 3,105
  • 8
  • 49
  • 87

6 Answers6

6

There are two problems here.

First one: You are defining your functions wrong.

firstMethod = () => (event) => {
    console.log("button clicked")
    this.secondMethod();
}

Like this, you are returning another function from your function. So, it should be:

firstMethod = ( event ) => {
    console.log("button clicked")
    this.secondMethod();
}

Second: You are not using onClick handler, instead invoking the function immediately.

<span onClick={this.firstMethod()}>Click me</span>

So, this should be:

<span onClick={() => this.firstMethod()}>Click me</span>

If you use the first version, yours, when component renders first time your function runs immediately, but click does not work. onClick needs a handler.

Here, I totally agree @Danko's comment. You should use this onClick with the function reference.

<span onClick={this.firstMethod}>Click me</span>

With this method, your function is not recreated every time your component renders since it uses your handler function with its reference. Also, no struggle writing the handler manually.

Lastly, if you define your functions as an arrow one, you don't need to .bind them.

Here is the working code.

class App extends React.Component {

  firstMethod = () => {
    console.log("button clicked")
    this.secondMethod();
  }

  secondMethod = () =>
    console.log("this is never called!")

  // or maybe even better not using an arrow function for the
  // second method since it is already bound to `this` since we
  // invoke it from the firstMethod. For details, look the comments please.

  /* secondMethod() {
    console.log("this is never called!")
  } */
 

  render() {
    return(
      <span onClick={this.firstMethod}>Click me</span>
    )
  }
}

const rootElement = document.getElementById("root");
ReactDOM.render(<App />, rootElement);
<script src="https://cdnjs.cloudflare.com/ajax/libs/react/15.1.0/react.min.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/react/15.1.0/react-dom.min.js"></script>
<div id="root"></div>
devserkan
  • 16,870
  • 4
  • 31
  • 47
  • Actually, even in your final solution, there is room for improvement: `secondMethod` should be written as a plain method property, not arrow function. – amankkg Aug 07 '18 at 19:31
  • Yeah, sure. Since `firstMethod` is an arrow function already (e.g. it is already "bound" to `this`), every method call inside that method will be "local" to `this`. – amankkg Aug 07 '18 at 20:05
  • @amankkg, thanks. You mean automatic binding since we do not bind it explicitly here? If so, what is the downside of using arrow function instead of using class method? – devserkan Aug 07 '18 at 21:30
  • Yes, I mean automatic binding: babel transpiles arrow function into plain `.bind(this)` in the constructor. A downside is in redundant `bind` of `secondMethod`. – amankkg Aug 07 '18 at 21:38
  • 1
    Other than the situations (some of them arguable) talked about in this medium article: https://medium.com/@charpeni/arrow-functions-in-class-properties-might-not-be-as-great-as-we-think-3b3551c440b1 – devserkan Aug 07 '18 at 21:38
  • Ok than @amankkg, I got your point. Though, I'm an arrow function guy as a learner :) But in this particular situation you are right, arrow function is unnecessary. – devserkan Aug 07 '18 at 21:40
  • 1
    Performance win is too small here, tbh. A much bigger win here is a mental model of code being explicit, not confusing. Because when I see arrow function class property I keep in mind that this function is designed to be invoked outside of this component. Otherwise, it is a tech debt, that complicates understanding of the intents behind this code. – amankkg Aug 07 '18 at 21:49
  • @amankkg, very good point. I can argue using an arrow function or binding it in the constructor explicitly, but not this. Great explanation! – devserkan Aug 07 '18 at 21:54
  • Thanks again. I read that if there is no transpile native one is fast enough as the regular functions? – devserkan Aug 07 '18 at 22:12
  • There is a good comparison between them in the blog post you mention above. Which corrects some of my mistakes regarding performance diffs. So, IMO, for a function that's going to be passed somewhere TL;DR is to use arrow function over explicit bind in a constructor (i.e. readability > performance here). @bind decorator might change it in the future. – amankkg Aug 08 '18 at 13:30
2

Instead of firstMethod = () => (event) try firstMethod = (event) => and instead of secondMethod = () => (event) => { try secondMethod = (event) => {

Danko
  • 1,819
  • 1
  • 13
  • 12
  • 1
    and `onClick` event/prop should not be called with '()'. it should look like this: `Click me` – Danko Jul 25 '18 at 21:35
  • 1
    I totally missed that one tough I used this method in my all codes. Thanks @Danko pointing that out. – devserkan Jul 25 '18 at 21:52
  • It's great that you have sorted this out now :). Please upvote this answer if it was helpful to you – Danko Jul 25 '18 at 22:05
1

Your second method returns a new function, which is redundant.
Also, your second method can be not bound, since the first method has the context already.

secondMethod = () => (event) => { ... } should be secondMethod(evnt) { ... }

Here is the working and optimized example https://codesandbox.io/s/pkl90rqmyj

amankkg
  • 4,503
  • 1
  • 19
  • 30
1

The bad news is that you can't bind arrow functions because they're lexically bound. See:

Can you bind arrow functions?

The good news is that "lexical binding" means that they should already have App as their this, i.e. it should would without explicitly binding. You'd likely redefining them as undefined, or some other odd thing by treating them this way in the constructor.

Joshua R.
  • 2,282
  • 1
  • 18
  • 21
1

Try this, it works for me.

  firstMethod = () => {
    console.log("click handler for button is provided")
    return (event) => this.secondMethod(event);
  }

  secondMethod = (event) => {
    console.log("This is being called with", event);
  }
Kamalakannan J
  • 2,818
  • 3
  • 23
  • 51
1

Can you check this way using this url: https://codesandbox.io/s/q4l643womw I think you're using wrong bind methods but here you can see an example

class App extends Component {
  constructor() {
     super();
     this.state = {
       num: 1,
     };
     this.firstMethod = this.firstMethod.bind(this);
     this.secondMethod = this.secondMethod.bind(this);
  }

  firstMethod() {
    this.secondMethod();
  }

  secondMethod() {
    this.setState({
      num: 2
    });
  }

  render() {
    return (
      <div className="App">
        <button onClick={this.firstMethod}>click</button>
        <label>{this.state.num}</label>
      </div>
    );
  }
}