Pure render for tree data, fixes React Canary useMemo/useState StrictMode#5835
Pure render for tree data, fixes React Canary useMemo/useState StrictMode#5835
Conversation
|
Build successful! 🎉 |
| let node = originalMap.get(key); | ||
| if (!node) { | ||
| return items; | ||
| return {items, mappy: originalMap}; |
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| let initialNodes = useMemo(() => buildTree(initialItems), []); | ||
| let [items, setItems] = useState(initialNodes); | ||
| let [tree, setItems] = useState<{items: TreeNode<T>[], mappy: Map<Key, TreeNode<T>>}>(() => buildTree(initialItems)); |
| setItems(items => { | ||
| let nodes = buildTree(values, parentKey); | ||
| setItems(({items, mappy: originalMap}) => { | ||
| let {items: nodes, mappy: newMap} = buildTree(values, parentKey, originalMap); |
There was a problem hiding this comment.
won't this still result in originalMap being mutated? buildTree only creates a new map if acc is not provided, otherwise it reuses it.
There was a problem hiding this comment.
not quite sure I follow here. We are passing originalMap here to buildTree to build the newMap for the parentKey === null case. If we didn't create pass it, buildTree would create a new map that would then only have the new nodes that are being inserted here instead of the full modified tree map since values is just the new values being inserted
modifying the originalMap here shouldn't matter right since we are now storing the map in state and accessing the previous map inside the setState call?
There was a problem hiding this comment.
yeah here https://github.com/adobe/react-spectrum/pull/5835/files#diff-c1a25c31946ec151047ebc265b0f5cf0971d990a3407c0dd66b7beecb1be4b24R151 we mutate the map that gets passed into buildTree, thereby mutating the map stored in state.
|
Build successful! 🎉 |
|
Build successful! 🎉 |
this avoids the case where we may inadvertantly modify the previous map directly which could be problematic in a double render
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
## API Changes
unknown top level export { type: 'any' } |
6 participants
Closes
Fixes test failures on main
The underlying root cause was that React made a PR which turned on double invocation for useState in React StrictMode, and it changed the value that useMemo returns to discard the second result of the run instead of the first.
This was a bug for us to fix because if we were a pure render then it should not matter which result they kept, it'd be the same. This puts our map into state so that we aren't mutating it anymore.
See facebook/react#28249 (comment)
✅ Pull Request Checklist:
📝 Test Instructions:
Smoketest both Canary and 18
🧢 Your Project: