-
-
Notifications
You must be signed in to change notification settings - Fork 15.2k
Description
What docs page needs to be fixed?
- Section: Redux Essentials Tutorial
- Page: Part 6: Performance and Normalization — Memoizing Selector Functions
What is the problem?
https://redux.js.org/tutorials/essentials/part-6-performance-normalization#memoizing-selector-functions
The code example demonstrating selectPostsByUser memoization contains two potential misleading issues, as confirmed by @markerikson after I asked about it:
const state1 = getState()
// Output selector runs, because it's the first call
selectPostsByUser(state1, 'user1')
// Output selector does _not_ run, because the arguments haven't changed
selectPostsByUser(state1, 'user1')
// Output selector runs, because `userId` changed
selectPostsByUser(state1, 'user2')
dispatch(reactionAdded())
const state2 = getState()
// Output selector does not run, because `posts` and `userId` are the same
selectPostsByUser(state2, 'user2')
// Add some more posts
dispatch(addNewPost())
const state3 = getState()
// Output selector runs, because `posts` has changed
selectPostsByUser(state3, 'user2')-
Technical Flaw: The line following dispatch(reactionAdded()) incorrectly asserts that the output selector does not run. Since reactionAdded mutates a post (via Immer), the state.posts reference changes, meaning the input selector (selectAllPosts) returns a new reference, causing the output selector to run. (Even if the result of the output selector happens to be the same with the previously memoized result, and thus NOT returning the new reference.)
-
Pedagogical Flaw: The example lacks a crucial test case that the "Improving Render Performance" sub-section is based on: demonstrating selector stability against actions that modify unrelated state (e.g., fetching notifications).
What should be changed to fix the problem?
The code block should be replaced with an enhanced version that accurately reflects the behavior of both dependent and non-dependent state changes, as follows:
Correct the assertion for reactionAdded() (the output selector runs).
Add a test case for dispatch(fetchNotifications()) (the output selector does not run).
Proposed update of the code example block
const state1 = getState()
// Output selector runs, because it's the first call
selectPostsByUser(state1, 'user1')
// Output selector does _not_ run, because the arguments haven't changed
selectPostsByUser(state1, 'user1')
// Output selector runs, because `userId` changed
selectPostsByUser(state1, 'user2')
dispatch(reactionAdded())
const state2 = getState()
// Output selector runs, because the `posts` has changed
selectPostsByUser(state2, 'user2')
// Add some more posts
dispatch(addNewPost())
const state3 = getState()
// Output selector runs, because `posts` has changed
selectPostsByUser(state3, 'user2')
// Fetch notifications
dispatch(fetchNotifications())
const state4 = getState()
// Output selector does not run, because `posts` and `userId` are the same
selectPostsByUser(state4, 'user2')Technical note
It is important to confirm that the output selector function itself runs after reactionAdded or addNewPost, because the posts reference input changes. While Reselect uses a default equality check to prevent the output reference from changing if the calculated result is identical, the function itself must first execute to perform that check. Therefore, stating that the selector runs is the most accurate description of the internal memoization logic in this context, even if the eventual component re-render might be prevented by a more complex equal function (which isn't the primary focus here). This was my thought process, although at first I explicitly stated this in the example, it cluttered the example and came to the conclusion that it could potentially cause more confusion rather than helping the reader. Any edit recommendations or corrections are appreciated!