BindAll: Class constructor cannot be invoked without 'new'

[icfantv]

This is probably the same issue as #26 but I have a repo that reproduces the issue.

Repo: https://github.com/icfantv/lodash-decorator-issue
Just run yarn install followed by yarn test

If you run tsc you can see the generated output in the build directory. I freely admit I don't understand decorators and the generated code well enough to diagnose this as an issue w/ this lib or perhaps ts-jest. Happy to provide assistance if/where I can.

[icfantv]

Digging further, it appears the decorator works fine in production which would seem to indicate where to look further. Going to keep digging.

[icfantv]

Update from above reference: I can reproduce this with react-testing-library as well but am still not yet convinced it's an issue with this library.

[icfantv]

Another update: I tried with core-decorators instead of lodash-decorators and the tests pass just fine. So now I'm back to square one and can't say for sure that there's not an issue w/ lodash-decorators.

[steelsojka]

This could be an issue with lodash-decorators implementation. core-decorators swaps out the method on the prototype with a getter that binds upon accessing the property on the instance. lodash-decorators tries to replace the constructor with one that binds upon construction (I don't think this works with all transpilers). I like the core-decorators approach to this. I'll try and find some time to create a similar implementation using lodash.

[icfantv]

@steelsojka I have a concern about the core-decorators approach and I'm not convinced it has to be this way (but my JS-fu is not strong): It's going to execute that get() every time a bound method is executed which seems very non-performant (haven't tested this) and not a good design (maybe...).

Whereas, your solution is what React even recommends for <T extends React.Component> in that one should bind all non-static methods that access this (except for render() which React does automatically) in the constructor.

[steelsojka]

The problem with my approach is that we hi jack the constructor and invoke the original with .apply. This doesn't jive with some transpilers and produces the error you are getting. We won't call the getter everytime, only on the first access. In that getter we define the property on the instance bound. So I don't think there will be much change in behavior. I'll make sure to test these scenarios.

[steelsojka]

@icfantv So the error occurs when using native classes.

class MyClass {}
MyClass.apply({});

I'll try and find a way to make it work.

[steelsojka]

@icfantv I think I have a fix. I will try and get it out in the next day or so.

[icfantv]

@steelsojka awesome and thanks! glad to hear it and happy to take a look.

[steelsojka]

Since this is a breaking change in behavior, this is in version 6.0.0.