-
Notifications
You must be signed in to change notification settings - Fork 49
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
Create a HoC as an alternative to the mixin #37
base: master
Are you sure you want to change the base?
Conversation
I think it will be great to have a HoC version that people can use as an alternative to the mixin. Would it be possible to refactor the PR so that it shares most of its logic with the mixin, instead of having the logic in two places? Also, can we depend on a less strict version of react than |
Fixed those two things! My initial pull request was more of a draft, hence those two issues. |
@@ -25,5 +25,8 @@ | |||
"babel-eslint": "^4.1.7", | |||
"eslint": "^0.22.1", | |||
"mocha": "^2.2.5" | |||
}, | |||
"peerDependencies": { | |||
"react": "^0.14.0 || ^15.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this work with react 0.13 as well? This library was first published when react 0.13 was the latest, so it would be good if merging this doesn't make older projects incompatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know, it should work with 0.13. The HoC doesn't do much more than change the API from the mixin. I'll be updating the version again in my next update.
For those who didn't know (like me): HoC = Higher-order Component. |
Pushing updates to my fork now |
import {ensure} from './util'; | ||
import {update, unmount} from './Mixin'; | ||
|
||
// Mixin for RethinkDB query subscription support in React components. You'll |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HoC, not mixin
This is coming along great! I made a few more comments, but otherwise I think it's fine. We'll probably want to add or modify an example to use ES6 classes, but that doesn't need to happen in this PR. |
Since mixins don't work with React.Component and are likely on the depreciation path, a HoC is a better interface option for React. What do you think?