-
-
Notifications
You must be signed in to change notification settings - Fork 15.2k
Update ComputingDerivedData.md #1516
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
Conversation
docs/recipes/ComputingDerivedData.md
Outdated
| In the example below `mapStateToPropsFactory` creates a new `getVisibleTodos` selector, and returns a `mapStateToProps` function that has exclusive access to the new selector: | ||
|
|
||
| ```js | ||
| const mapStateToPropsFactory = () => { |
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.
I’m a bit concerned we are calling the same concept “factory” and “creator” on the same page.
Maybe we could change that to makeGetVisibleProps(), makeMapStateToProps()?
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.
Yes, fair point.
ab364c0 to
d843d4b
Compare
docs/recipes/ComputingDerivedData.md
Outdated
| export default VisibleTodoList | ||
| ``` | ||
|
|
||
| A selector created with `createSelector` only returns the cached value when its set of arguments is the same as its previous set of arguments. If we alternate between rendering `<VisibleTodoList listId="1" />` and `<VisibleTodoList listId="2" />`, the shared selector will alternate between receiving `{listId: 1}` and `{listId: 2}` as its `props` argument. This will cause the arguments to be different on each call, so the selector will always recompute instead of returning the cached value. We'll see how to overcome this limitation in the next section. |
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.
Nit: can you please use ’ instead '
04e0840 to
a65b81f
Compare
docs/recipes/ComputingDerivedData.md
Outdated
| ```js | ||
| const makeMapStateToProps = () => { | ||
| const getVisibleTodos = makeGetVisibleTodos() | ||
| return (state, props) => { |
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.
Can you change it here, too?
const mapStateToProps = ...
return mapStateToProps|
Great work. Will merge after this last nit. Thank you very much. |
a65b81f to
99f6984
Compare
docs/recipes/ComputingDerivedData.md
Outdated
| ### Motivation for Memoized Selectors | ||
|
|
||
| Let's revisit the [Todos List example](../basics/UsageWithReact.md): | ||
| > The examples in this section are based on the [Redux Todos List example](http://redux.js.org/docs/basics/UsageWithReact.html). |
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.
What is the reason for this link change?
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.
Oh, I didn't do it on purpose, it's because of a copy paste from the reselect docs. I'll switch it back.
45acb4d to
91b03f6
Compare
91b03f6 to
a982b6f
Compare
|
Awesome, I reckon this is ready – really useful resource! |
Here's an update for the Computing Derived Data doc that I have been tinkering with for a couple of weeks. To be honest I found it pretty hard to write, any criticisms or suggested improvements are very welcome!