andreypopp / autobind-decorator

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

Autobind doesn't bind functions to class prototype #40

Closed jamby closed 7 years ago

jamby commented 7 years ago

So while writing tests, I have a component:

import React, { Component } from 'react';
import autobind from 'autobind-decorator';

@autobind
export default class NavBar extends Component {
  handleClick() {
    console.log("Clicked!");
  }

  render() {
    return (
      <div>
        <a role="button" onClick={this.handleClick}>
          Click this
        </a>
      </div>
    );
  }
}

And then inside the my tests for Enzyme, using Karma/Jasmine:

describe("COMPONENTS: <NavBar />", function() {
  it("simulates click event for acceptingChats false", function() {
    const handleClick = spyOn(NavBar.prototype, 'handleClick');
    const rendered = shallow(<NavBar {...props} />);
    rendered.find('a').simulate('click');
    expect(handleClick).toHaveBeenCalled();
  });
});

However, this doesn't work because autobind doesn't actually put them inside the prototype of the class. Was wondering if there was a possibility for it to be bound to the prototype when done this way or just to each function in particular.

stevemao commented 7 years ago

What's the use case of putting them on the prototype?

jamby commented 7 years ago

In my case, I'm trying to spy on the component's function via the prototype before the component is rendered (for testing), so I can make sure it's called. Not sure if there is any other use case, but that'st he one I've ran into.

stevemao commented 7 years ago

I think the point of the decorator is to not overwrite the original prototype methods. But when you use them, it uses the own methods on the object. There are also performance reasons.

In your use case, I think it's very easy to create an instance and spy on it. Well probably not true. But anyway, I don't feel this is right way to write a test.

As stated in the README, Class instance properties is probably the better pattern in many cases.

abdennour commented 7 years ago

@jamby : i had the same issue , and @babel-autobind can solve your issue by two ways:

1. Either, you keep @autobind decorator , and bind again on unit-tests using babel-autobind :

//no need to alter NavBar component, but you will modify unit-tests following this way. 
import {Autobind} from 'babel-autobind';

const _Navbar = Autobind(NavBar);

describe("COMPONENTS: <NavBar />", function() {
  it("simulates click event for acceptingChats false", function() {
    const handleClick = spyOn(_Navbar.prototype, 'handleClick');
    const rendered = shallow(<_Navbar {...props} />);
    rendered.find('a').simulate('click');
    expect(handleClick).toHaveBeenCalled();
  });
});

2. Or migrate totally to babel-autobind :


import React, { Component } from 'react';
import {Autobind} from 'babel-autobind';

@Autobind
 class NavBar extends Component {
  handleClick() {
    console.log("Clicked!");
  }

  render() {
    return (
      <div>
        <a role="button" onClick={this.handleClick}>
          Click this
        </a>
      </div>
    );
  }
}

export default NavBar;

and no need to change anything in unit-tests if you migrate.


Just FYI, this package was designed and implemented today after facing this issue: building was passed and tests covers 100% of code.. Please, accept this package from me as gift 🎁 🎁 🎁 🎁 🎁 🎁

wesleysmyth commented 7 years ago

@abdennour - thanks for creating this package! I just tried it out in my jasmine tests and found that wrapping the component with Autobind still didn't solve the issue of spying on a classes' prototype method. But of course it's still super new, so maybe I can help look into the problem.

abdennour commented 7 years ago

@wesleysmyth :

Then, if it does not still work ,, please , try the following:

wesleysmyth commented 7 years ago

Awesome, @abdennour - will give it a shot

stevemao commented 7 years ago

I think the point of the module is not to mutate the prototype. class instance property or https://github.com/andreypopp/autobind-decorator/issues/40#issuecomment-264727287 might be better alternatives.

abdennour commented 7 years ago

You are Welcome @wesleysmyth