andreypopp / autobind-decorator

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

Break sinon.spy(Component.prototype, 'method') #47

Open noru opened 7 years ago

noru commented 7 years ago

When using '@autobind' on a component, then in tests sinon.spy(Component.prototype, 'someMethod') will yield an error:

TypeError: Attempted to wrap undefined property undefined as function
...

Sorry, currently I can't provide more detail, but I'm sure @autobind is the only variable between success/error in tests.

damianobarbati commented 7 years ago

+1 same problem with jest


        const instance = component.instance();
        const spy = jest.spyOn(Component.prototype, 'compute');

        component.find('button').simulate('click');
        component.update();

        expect(spy).toBeCalled();
stevemao commented 7 years ago

98968ee might fixed this. Please try the latest master branch

stevemao commented 6 years ago

Please comment if doesn't work for you

job13er commented 6 years ago

Just in case anyone else happens upon this, I'm fairly certain autobind-decorator is incompatible with sinon (at least with .stub()). It appears sinon uses descriptor.value to check if something is a method or not: https://github.com/sinonjs/sinon/blob/v4.3.0/lib/sinon/stub.js#L30-L34.

As a result, the descriptor.get() that autobind-decorator creates here: https://github.com/andreypopp/autobind-decorator/blob/v2.1.0/src/index.js#L75-L95 and here: https://github.com/andreypopp/autobind-decorator/blob/v2.1.0/src/index.js#L83-L92

are always considered a non-function properties.

I didn't dig into sinon.spy as much, but I don't think it was working on @autobind methods either.

damianobarbati commented 6 years ago

@job13er just declare your class methods as class properties.

job13er commented 6 years ago

@damianobarbati Thanks. That's always an option, but since I was looking at autobind-decorator, it seems obvious I already have reasons not to want to use class properties, or I would have had no need for autobind-decorator in the first place.

EDIT: Your comment did make me take a second look at why we weren't using class properties in the first place though, so thank you :)

stevemao commented 6 years ago

@job13er Maybe we should add this to the docs

stoically commented 5 years ago

Was running into sinon stubs clashing with autobind as well - is there a chance to reopen this issue or is it considered WONTFIX?