-
Notifications
You must be signed in to change notification settings - Fork 672
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
Document per-component memoization with React Redux #79
Comments
Thanks @gaearon. There are a couple of other things I'd like to see too:
If someone wants to take this on then I would appreciate it. Make a comment here so people know you are working on it. If there are no takers I will pick it up at the start of next week. |
@tgriesser has taken on describing the new per-instance memoization feature, but the other two bullet points in the last comment still need attention. |
I am working on this now. |
@ellbee @tgriesser Do you have an update on the documentation? I'm trying to implement this into my app but the API is not very clear. Currently have it setup like this: //./Selectors/NodeSelector
import {createSelector} from 'reselect'
const idSelector = (state, props) => props.id;
const treeSelector = state => state.project.get('tree');
export default createSelector(treeSelector, idSelector, (tree, id) => {
id = String(id);
return tree.has(id) ? tree.get(id).toJS() : {};
}); //./Components/Node
export default connect(() => {
return NodeSelector;
})(Node); The API for the <Node id={1} />
<Node id={2} />
<Node id={3} /> In my current implementation I'm still seeing unnecessary updates. |
@bramdevries the issue here is //./Selectors/NodeSelector
import {createSelector} from 'reselect'
const treeItemSelector = (state, props) => (
state.project.getIn(['tree', '' + props.id], Immutable.Map());
)
export default createSelector(treeItemSelector, (treeItem) => {
return treeItem.toJS()
}); So the |
@tgriesser I see, thanks for the snippet. However, I'm not seeing the memoization take place, it's still rendering all the @DragSource(DragTypes.NODE, source.spec, source.collect)
@DropTarget(DragTypes.NODE, target.spec, target.collect, {
arePropsEqual: shallowEqual,
})
class Node extends Component {}
export default connect(() => NodeSelector)(Node); If I remove the |
Turns out I completely missed this part of the document that explains this concept very well: https://github.com/reactjs/reselect/blob/master/README.md#sharing-selectors-across-multiple-components Working now. |
You didn't miss it, I've only just put it up! 🙂 Glad to hear that it helped! |
The docs now say:
If you jump right to this section without reading the immediately preceding section, this sentence make it seem like this rule applies to all selectors. Should we clarify that this only applies to a selector that's called with changing |
Hi @courthead. I agree, it can't hurt to clarify if it is potentially unclear. If you'd like to submit a PR, that would be great! |
React Redux added per-instance memoization in 4.3.0.
This approach, originally explained here (although the API changed), allows significant perf wins for Reselect when computed state props depend on component’s own props, as those are now memoized per instance.
I think this approach is worth explaining both here and in the Computing Derived Data recipe. I don’t have time to contribute this, but I’m sure it will be a very valuable addition.
cc @tgriesser who implemented the feature in React Redux
The text was updated successfully, but these errors were encountered: