andreypopp / autobind-decorator

Decorator to automatically bind methods to class instances
MIT License
1.45k stars 66 forks source link

The autobind doesn't work occasionally #57

Open sailorT opened 7 years ago

sailorT commented 7 years ago

Hi andreypopp, In my react native project, the autobind doesn't work occasionally. I find the root cause is the origin function would be return when its 'get' has been called twice. I think the bind result should be return in this case.
Please help to review this PR. Thanks!

stevemao commented 7 years ago

Take a wild guess... Is

Object.defineProperty(this, key, {
         value: boundFn,
         configurable: true,
         writable: true
});

running async in react native? Or somewhere in the outter get() function may be async? Because the second time you try to access it, it goes to the inner get(), which just returns the bound function.

sailorT commented 7 years ago

I understand your mean, and I couldn't point out the root cause for it. But I think this protect is better. If you can find the root cause, it would be great! BTW, this error would not be happen when the remotely debug opened!

lprhodes commented 7 years ago

This fixed the same issue for me.

In my case I'm using relay. Methods would no longer be bound after I performed a mutation that caused the component tree to update cells in a react-native FlatList. (Only on Android, current versions of all modules)

lprhodes commented 7 years ago

This breaks calling methods on super for me. The method on the current class gets called again recursively instead of the method on super

e.g.

import React, { Component } from 'react'
import { Text, TouchableOpacity } from 'react-native'
import autobind from 'autobind-decorator'

class ParentComponent extends Components {

  @autobind
  myMethod () {
    console.log('Parent')
  }
}

class ChildComponent extends ParentComponent {

  @autobind
  myMethod () {
    console.log('Child')
    super.myMethod()
  }

  render () {
    return (
      <TouchableOpacity onPress={this.myMethod}>
        <Text>Tap Me</Text>
      </TouchableOpacity>
    )
}

export default ChildComponent
isilher commented 6 years ago

In our project we also encountered the issue this PR was made to fix. We applied @autobind to every Class though (would not do that again in future projects as recomended) and the suggested fix in this PR broke our build in a lot of places. It turned out componentWillUnmount was being called in an infinite loop. The following code snippet may not win a beauty contest, but did solve our problem. Perhaps it can be of help to you or others struggling with the same issue.

...
  return {
    configurable: true,
    get() {
      if (definingProperty || this === target.prototype || this.hasOwnProperty(key)
        || typeof fn !== 'function') {
        const lifecycleMethods = [
          'constructor',
          'componentWillMount',
          'componentDidMount',
          'componentWillReceiveProps',
          'shouldComponentUpdate',
          'componentWillUpdate',
          'render',
          'componentDidUpdate',
          'componentWillUnmount',
        ];
        if (!lifecycleMethods.includes(key) && this.hasOwnProperty(key)) {
          return this[key];
        }
        return fn;
      }
      ...
    }
  };
stevemao commented 6 years ago

Can we create a unit test against this?

mieszko4 commented 6 years ago

@stevemao Not sure if it is easy to unit test that, I will try to take a look next week

xanderdeseyn commented 6 years ago

Just ran into this issue today, so it's still out there.

stevemao commented 5 years ago

@N1ghtly please write a failing unit test to illustrate this issue.