Skip to content
This repository has been archived by the owner on Sep 29, 2020. It is now read-only.

order of decorators causing some decorators not to apply #80

Closed
tommedema opened this issue May 21, 2016 · 8 comments
Closed

order of decorators causing some decorators not to apply #80

tommedema opened this issue May 21, 2016 · 8 comments

Comments

@tommedema
Copy link

When combining e.g. autobind with react-css-modules the autobind decorator stops working, unless it is specified as the last decorator (closest to the class declaration). I.e. this makes autobind work:

@cssModules(Bootstrap, { allowMultiple: true })    
@autobind

But cssModules to fail. The following will make @autobind have no effect.

@autobind
@cssModules(Bootstrap, { allowMultiple: true })

Is this because of how @cssModules was implemented, or is @autobind also playing a role in this issue?

@jayphelps
Copy link
Owner

@cssModules actually wraps your class in a Higher-Order Component (HOC), which means in the first example, you're actually @autobinding the HOC, not your original component. The order in which you apply decorators very often does matter, which is a necessary caveat to support cool things like HOCs.

@jayphelps
Copy link
Owner

jayphelps commented May 21, 2016

But cssModules to fail

Sorry, I didn't see that it causes cssModules to fail when placed in the correct order. I don't immediately see why that would happen. Can you clarify what fail means? What happens?

@jayphelps jayphelps reopened this May 21, 2016
@tommedema
Copy link
Author

@jayphelps in this issue the author describes why it doesn't work when cssModules is not the last decorator: https://github.com/gajus/react-css-modules/issues/9

@jayphelps
Copy link
Owner

@tommedema right, perhaps I'm misunderstanding. Can you clarify: if you place @autobind as one that gets applied first to your class, does both @autobind and @cssModules work as expected? If yes, then things are working as-designed and my original comment applies. If not, I need more specifics about what is happening, errors you receive, etc.

@tommedema
Copy link
Author

@jayphelps if I place @autobind as the one that gets applied first to my class, only @autobind works, @cssModules does not work. There is no error, but cssModules is simply not applied to my class. I now suspect this is because of how cssModules is implemented, so perhaps this issue can be closed.

@jayphelps
Copy link
Owner

jayphelps commented May 23, 2016

@tommedema I just tried it and the combination of the two work correct for me. I used the latest versions of both libraries, with babel v6.

I dug into the code for react-css-modules and found something that is possibly suspicious. If you apply @autobind on the entire class, that obviously autobinds every method, even ones that don't necessarily need to be autobound in the React usecase like render(). In the case of react-css-modules, it checks whether what you pass it is a React Component class or a functional-component by checking for the existence of a render() function on the prototype. core-decorators is actually clever enough to account for this and do the expected thing, but it's totally possible that there's yet enough edge case we're missing.

So to further investigate, can you let me know what version of core-decorators you're using? Some of these fixes were fairly recent, 0.12.2 is the latest, please be sure you've upgraded to that version and then test it.

If you're on the latest version, can you copy and paste you're entire npm dependencies and devDependencies? There a number of react and babel related tools that do fairly hacky things for dev tools purposes, like hot reloading etc. Seeing what you're using will help me be confident it isn't some other dependency unknowingly cause the issue. e.g. @autobind doesn't work with react-hot-reloader <= 1.3.0 but I don't think that's related to the issue you're having since the symptoms are different.

@jayphelps
Copy link
Owner

jayphelps commented May 23, 2016

Just found that react-css-modules also extends your passed in class directly and invokes super.render() which might actually have not worked as expected in previous core-decorators versions due to the same prototype-lookup issues, so be 100% sure you're on [email protected] 😄

Also--in case you're blocked on this, just want to clarify that you can just use @autobind on each individual method you need bound, instead of using @autobind on the entire class itself.

@jayphelps
Copy link
Owner

Closing due to inactivity, but if this is still an issue please do let me know with the requested info above.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants