-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,107 @@ | ||
import React from 'react'; | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. HoC, not mixin |
||
// generally want to use DefaultHoC or PropsHoC, which use BaseHoC to | ||
// create more usable versions. | ||
// | ||
// Along with your component, you should pass an observe(props, state) function that | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove |
||
// returns an object mapping query names to QueryRequests. See | ||
// QueryRequest.js for the API. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this comment might be misleading, since the client shouldn't define There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So this would be fine then? // In your component, you should pass an observe(props, state) function that
// returns an object mapping query names to QueryRequests. See
// QueryRequest.js for the API. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, although something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha.
|
||
// | ||
// In the child component, you will have access to this.props.data, which is an | ||
// object mapping from the same query names returned in observe() to the | ||
// results of each query as an QueryResult. See QueryResult.js for the | ||
// API. | ||
// | ||
// Here is a simple example of the mixin API: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. HoC, not mixin |
||
// const observe = (props) => ({ | ||
// turtles: new QueryRequest({ | ||
// query: r.table('turtles'), | ||
// changes: true, | ||
// initial: [], | ||
// }), | ||
// }); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indentation is a bit off |
||
// class App extends Component { | ||
// render() { | ||
// return <div> | ||
// {this.props.data.turtles.value().map(function(x) { | ||
// return <div key={x.id}>{x.firstName}</div>; | ||
// })}; | ||
// </div>; | ||
// }, | ||
// }; | ||
|
||
// BaseHoC(() => new Session())(observe)(App); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Normally someone would do |
||
|
||
export const BaseHoC = sessionGetter => observe => ChildComponent => class ReactRethinkDB extends React.Component { | ||
constructor(props) { | ||
super(); | ||
this.observe = observe; | ||
} | ||
|
||
componentWillMount() { | ||
const session = sessionGetter(this); | ||
this.runQuery = session.runQuery.bind(session); | ||
ensure(session && session._subscriptionManager, 'Must define Session'); | ||
ensure(this.observe, 'Must define observe()'); | ||
ensure(session._connPromise, 'Must connect() before mounting react-rethinkdb'); | ||
this._rethinkMixinState = {session, subscriptions: {}}; | ||
this.data = this.data || {}; | ||
update(this, this.props); | ||
} | ||
|
||
componentDidMount() { | ||
this._rethinkMixinState.isMounted = true; | ||
} | ||
|
||
componentWillUnmount() { | ||
unmount(this); | ||
this._rethinkMixinState.isMounted = false; | ||
} | ||
|
||
componentWillUpdate(nextProps) { | ||
if (nextProps !== this.props) { | ||
update(this, nextProps); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand that props passed to this HoC will get copied to the child component, so it makes since we'd use the HoC's props. But is state ever set anywhere? It doesn't look like the HoC is accessing the child component's state, and there's no code modifying the state in the HoC. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, looks like you're right. Removing state from the HoC. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good. To clarify, if someone wants to use state to modify some aspect of the query, they'll have to do so at a component above the one defining the query and pass it down as props? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right! |
||
} | ||
|
||
render() { | ||
return <ChildComponent data={this.data} runQuery={this.runQuery} {...this.props} />; | ||
} | ||
}; | ||
|
||
// Within ChildComponent, you can then run queries like this: | ||
// var App = React.createClass({ | ||
// handleSubmit: function(event) { | ||
// event.preventDefault(); | ||
// var { runQuery } = this.props | ||
// var nameInput = this.refs.firstName; | ||
// var query = r.table('turtles').insert({firstName: nameInput.value}); | ||
// nameInput.value = ''; | ||
// runQuery(query); | ||
// }, | ||
|
||
// render: function() { | ||
// var turtleDivs = this.data.turtles.value().map(function(x) { | ||
// return <div key={x.id}>{x.firstName}</div>; | ||
// }); | ||
// return <div> | ||
// <form onSubmit={this.handleSubmit}> | ||
// <input type="text" ref="firstName" /> | ||
// <input type="submit" /> | ||
// </form> | ||
// {turtleDivs} | ||
// </div>; | ||
// }, | ||
// }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move this example to the comment at the top, so all of the documentation is in one place There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should probably be an ES6 class instead of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the PropsHoC example be moved up as well, or is that an edge case that should stay at the bottom? |
||
|
||
// HoC that uses rethink session from props. For example: | ||
// class MyComponent extends Component { | ||
// ... | ||
// }); | ||
// var session = new Session(); | ||
// React.render(<MyComponent rethinkSession={session} />, mountNode); | ||
export const PropsHoC = name => BaseHoC(component => component.props[name]); |
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.
For clarity below, I'd prefer
import Mixin from './Mixin'
and callingMixin.update(...)
andMixin.unmount(...)
below. If that requires refactoring Mixin, then it's not a big deal, don't worry about it.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.
Semantically, I'd say that exporting these methods as part of the mixin default export is a bit misleading. These methods are also not tied directly to the mixin. The functions don't belong to the mixin any more than they belong to the HoC at this point, so It would probably make the most sense to put those methods in a different file entirely, like 'updateComponent.js' or something like that. Then both the HoC and mixin could pull from that file.
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.
Yeah that makes sense to refactor it so they both share the same code from a common base instead of HoC depending on Mixin. I'm fine deferring that to after this PR if that makes things simpler though.
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.
Sure. Let's defer it then.