Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get rid of mixins #2437

Closed
7 tasks done
rob0rt opened this issue Dec 8, 2015 · 11 comments
Closed
7 tasks done

Get rid of mixins #2437

rob0rt opened this issue Dec 8, 2015 · 11 comments
Labels
umbrella For grouping multiple issues to provide a holistic view

Comments

@rob0rt
Copy link
Contributor

rob0rt commented Dec 8, 2015

Right now, mixins are inhibiting a clean migration to es6 components (though a workaround exists with Decorators). Finding a way to get rid of them, or modify them so they don't need to be mixins, would be good.

This came up in conversation on #458.

Edit by @newoga

@rob0rt
Copy link
Contributor Author

rob0rt commented Dec 8, 2015

So the problem I see with this is the fact the mixin sets the propTypes as well as introduces other methods to the instance.

A possible solution would just be to move the methods into a helper class and then add the propType it adds directly to the propTypes in the parent component.

@rob0rt
Copy link
Contributor Author

rob0rt commented Dec 8, 2015

Another potential solution is making the mixins Decorators, and instead of adding the @mixin decorator to the class, you add something like @Stylable.

@rob0rt
Copy link
Contributor Author

rob0rt commented Dec 8, 2015

And a third potential solution is to just use @mixin. This doesn't seem like as bad of an idea to me the more I think about it, since it does essentially exactly what es5 mixins do now. There's not the new learning curve of dealing with custom Decorators, and it doesn't force a codebase-wide refactor of functionality (this is the biggest win with this technique in my opinion).

@oliviertassinari
Copy link
Member

And a third potential solution is to just use @mixin.

Adding the fact that we are working on another branch.
I think that's the best way to start the conversion.

@rob0rt
Copy link
Contributor Author

rob0rt commented Dec 8, 2015

Ah, my bad. I've got a one track mind at times 😛

@rob0rt
Copy link
Contributor Author

rob0rt commented Dec 8, 2015

My current PR uses the third technique. It doesn't exactly get rid of mixins but it does solve the problem of using them with ES6 classes.

@alitaheri
Copy link
Member

@BobertForever I kinda have an anti-pattern-phobia. I dislike mixins and love composition, orthogonality, and conventions. A subset of utility methods, HOCs and stuff will easily solve this issue. it's only a matter spending time and actually making it a reality. But I value productivity more than my opinions. Iterations and getting things done will teach your lessons beyond anything you can learn with just conversations. I propose this:

You go on ahead and migrate the classes the fastest way possible (I think that would be @mixin) and I will try to figure out a way to get rid of mixins once and for all :D

I truly appreciate the time you spent making this wish a reality. Thanks a lot 👍 👍

@rob0rt
Copy link
Contributor Author

rob0rt commented Dec 8, 2015

I agree about getting rid of mixins and I think your point with utility methods would be good (since a large portion of the mixin is just providing utility around the thing it added to the props). Looking through the mixins though I see a lot of them which rely heavily on the React methods, so moving them out to Utility methods would likely mean everywhere they were used, the class would have to implement those React methods and add in UtilityClass.[method]. That's more explicit and a lot of typing...

I guess potentially you could extend what used to be a mixin? Though that was brought up last time this conversation was had and doesn't really work when a class has multiple mixins.

@rob0rt
Copy link
Contributor Author

rob0rt commented Dec 9, 2015

So I ran into an issue with that library I was using, which I explain in PR #2451, and propose a temporary solution. It's basically my second suggestion from above, creating a custom decorator for what used to be a mixin.

@newoga newoga added the umbrella For grouping multiple issues to provide a holistic view label Jan 12, 2016
@newoga newoga mentioned this issue Feb 4, 2016
@alitaheri alitaheri modified the milestones: 0.15.0 Release, Migration to ES2015 Classes Feb 14, 2016
@alitaheri
Copy link
Member

Almost... 😍

@oliviertassinari oliviertassinari modified the milestones: 0.15.0 Release, 0.16.0 Release Mar 6, 2016
@oliviertassinari
Copy link
Member

This issue should have been closed ages ago. We do no longer use mixins 🎉 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
umbrella For grouping multiple issues to provide a holistic view
Projects
None yet
Development

No branches or pull requests

4 participants