From e391a8a980c50de823609f9cb9d8bc48911ad727 Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Sat, 2 Nov 2019 19:29:27 +0000 Subject: [PATCH 1/6] Change when node map is built --- .../material-ui-lab/src/TreeItem/TreeItem.js | 18 +++++-- .../material-ui-lab/src/TreeView/TreeView.js | 51 ++++++++++++++----- 2 files changed, 53 insertions(+), 16 deletions(-) diff --git a/packages/material-ui-lab/src/TreeItem/TreeItem.js b/packages/material-ui-lab/src/TreeItem/TreeItem.js index 93a3a9577f58d3..06747cf7945014 100644 --- a/packages/material-ui-lab/src/TreeItem/TreeItem.js +++ b/packages/material-ui-lab/src/TreeItem/TreeItem.js @@ -81,7 +81,8 @@ const TreeItem = React.forwardRef(function TreeItem(props, ref) { focusPreviousNode, handleFirstChars, handleLeftArrow, - handleNodeMap, + addNodeToNodeMap, + removeNodeFromNodeMap, icons: contextIcons, isExpanded, isFocused, @@ -223,10 +224,19 @@ const TreeItem = React.forwardRef(function TreeItem(props, ref) { React.useEffect(() => { const childIds = React.Children.map(children, child => child.props.nodeId); - if (handleNodeMap) { - handleNodeMap(nodeId, childIds); + if (addNodeToNodeMap) { + addNodeToNodeMap(nodeId, childIds); } - }, [children, nodeId, handleNodeMap]); + }, [children, nodeId, addNodeToNodeMap]); + + React.useEffect(() => { + if (removeNodeFromNodeMap) { + return () => { + removeNodeFromNodeMap(nodeId); + }; + } + return undefined; + }, [nodeId, removeNodeFromNodeMap]); React.useEffect(() => { if (handleFirstChars && label) { diff --git a/packages/material-ui-lab/src/TreeView/TreeView.js b/packages/material-ui-lab/src/TreeView/TreeView.js index 52996df0d5b4a3..f19a866b7ce11b 100644 --- a/packages/material-ui-lab/src/TreeView/TreeView.js +++ b/packages/material-ui-lab/src/TreeView/TreeView.js @@ -13,6 +13,16 @@ export const styles = { }, }; +function arrayDiff(arr1, arr2) { + if (arr1.length !== arr2.length) return true; + + for (let i = 0; i < arr1.length; i += 1) { + if (arr1[i] !== arr2[i]) return true; + } + + return false; +} + const defaultExpandedDefault = []; const TreeView = React.forwardRef(function TreeView(props, ref) { @@ -40,18 +50,21 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { const isTabable = id => tabable === id; const isFocused = id => focused === id; + const prevChildIds = React.useRef([]); React.useEffect(() => { - nodeMap.current = {}; const childIds = React.Children.map(children, child => child.props.nodeId); - nodeMap.current[-1] = { parent: null, children: childIds }; - - (childIds || []).forEach((id, index) => { - if (index === 0) { - firstNode.current = id; - setTabable(id); - } - nodeMap.current[id] = { parent: null }; - }); + if (arrayDiff(prevChildIds.current, childIds)) { + nodeMap.current[-1] = { parent: null, children: childIds }; + + (childIds || []).forEach((id, index) => { + if (index === 0) { + firstNode.current = id; + setTabable(id); + } + nodeMap.current[id] = { parent: null }; + }); + prevChildIds.current = childIds; + } }, [children]); const getLastNode = React.useCallback( @@ -248,7 +261,7 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { } }; - const handleNodeMap = (id, childrenIds) => { + const addNodeToNodeMap = (id, childrenIds) => { const currentMap = nodeMap.current[id]; nodeMap.current[id] = { ...currentMap, children: childrenIds, id }; (childrenIds || []).forEach(childId => { @@ -257,6 +270,19 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { }); }; + const removeNodeFromNodeMap = id => { + const map = nodeMap.current[id]; + if (map) { + if (map.parent) { + const parentMap = nodeMap.current[map.parent]; + const parentChildren = parentMap.children.filter(c => c !== id); + nodeMap.current[map.parent] = { ...parentMap, children: parentChildren }; + } + + delete nodeMap.current[id]; + } + }; + const handleFirstChars = (id, firstChar) => { firstCharMap.current[id] = firstChar; }; @@ -272,7 +298,8 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { focusPreviousNode, handleFirstChars, handleLeftArrow, - handleNodeMap, + addNodeToNodeMap, + removeNodeFromNodeMap, icons: { defaultCollapseIcon, defaultExpandIcon, defaultParentIcon, defaultEndIcon }, isExpanded, isFocused, From 24abda721ca6a420365bc9698ef9558b3f5301c4 Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Sat, 2 Nov 2019 19:36:32 +0000 Subject: [PATCH 2/6] Add defensive check --- packages/material-ui-lab/src/TreeView/TreeView.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/material-ui-lab/src/TreeView/TreeView.js b/packages/material-ui-lab/src/TreeView/TreeView.js index f19a866b7ce11b..f629852143531b 100644 --- a/packages/material-ui-lab/src/TreeView/TreeView.js +++ b/packages/material-ui-lab/src/TreeView/TreeView.js @@ -275,8 +275,10 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { if (map) { if (map.parent) { const parentMap = nodeMap.current[map.parent]; - const parentChildren = parentMap.children.filter(c => c !== id); - nodeMap.current[map.parent] = { ...parentMap, children: parentChildren }; + if (parentMap.children) { + const parentChildren = parentMap.children.filter(c => c !== id); + nodeMap.current[map.parent] = { ...parentMap, children: parentChildren }; + } } delete nodeMap.current[id]; From 5c7b6eba1fe24701011f1c77a95c6577238c994d Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Sat, 2 Nov 2019 19:41:06 +0000 Subject: [PATCH 3/6] More defensive checks --- packages/material-ui-lab/src/TreeView/TreeView.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/material-ui-lab/src/TreeView/TreeView.js b/packages/material-ui-lab/src/TreeView/TreeView.js index f629852143531b..123dd0042b079d 100644 --- a/packages/material-ui-lab/src/TreeView/TreeView.js +++ b/packages/material-ui-lab/src/TreeView/TreeView.js @@ -275,7 +275,7 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { if (map) { if (map.parent) { const parentMap = nodeMap.current[map.parent]; - if (parentMap.children) { + if (parentMap && parentMap.children) { const parentChildren = parentMap.children.filter(c => c !== id); nodeMap.current[map.parent] = { ...parentMap, children: parentChildren }; } From 343e805d3dedc447aee5b48862b60dd695051215 Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Sat, 2 Nov 2019 19:55:32 +0000 Subject: [PATCH 4/6] Default to empty array --- packages/material-ui-lab/src/TreeView/TreeView.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/material-ui-lab/src/TreeView/TreeView.js b/packages/material-ui-lab/src/TreeView/TreeView.js index 123dd0042b079d..1eefb9c1404ef9 100644 --- a/packages/material-ui-lab/src/TreeView/TreeView.js +++ b/packages/material-ui-lab/src/TreeView/TreeView.js @@ -52,7 +52,7 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { const prevChildIds = React.useRef([]); React.useEffect(() => { - const childIds = React.Children.map(children, child => child.props.nodeId); + const childIds = React.Children.map(children, child => child.props.nodeId) || []; if (arrayDiff(prevChildIds.current, childIds)) { nodeMap.current[-1] = { parent: null, children: childIds }; From 5ed6328d123d0263f94dc77973f8958918d7c35f Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Sat, 2 Nov 2019 20:01:39 +0000 Subject: [PATCH 5/6] Minor changes --- packages/material-ui-lab/src/TreeItem/TreeItem.js | 2 +- packages/material-ui-lab/src/TreeView/TreeView.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/material-ui-lab/src/TreeItem/TreeItem.js b/packages/material-ui-lab/src/TreeItem/TreeItem.js index 06747cf7945014..9a9878d319876f 100644 --- a/packages/material-ui-lab/src/TreeItem/TreeItem.js +++ b/packages/material-ui-lab/src/TreeItem/TreeItem.js @@ -223,7 +223,7 @@ const TreeItem = React.forwardRef(function TreeItem(props, ref) { }; React.useEffect(() => { - const childIds = React.Children.map(children, child => child.props.nodeId); + const childIds = React.Children.map(children, child => child.props.nodeId) || []; if (addNodeToNodeMap) { addNodeToNodeMap(nodeId, childIds); } diff --git a/packages/material-ui-lab/src/TreeView/TreeView.js b/packages/material-ui-lab/src/TreeView/TreeView.js index 1eefb9c1404ef9..286224cc78726b 100644 --- a/packages/material-ui-lab/src/TreeView/TreeView.js +++ b/packages/material-ui-lab/src/TreeView/TreeView.js @@ -56,7 +56,7 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { if (arrayDiff(prevChildIds.current, childIds)) { nodeMap.current[-1] = { parent: null, children: childIds }; - (childIds || []).forEach((id, index) => { + childIds.forEach((id, index) => { if (index === 0) { firstNode.current = id; setTabable(id); @@ -264,7 +264,7 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { const addNodeToNodeMap = (id, childrenIds) => { const currentMap = nodeMap.current[id]; nodeMap.current[id] = { ...currentMap, children: childrenIds, id }; - (childrenIds || []).forEach(childId => { + childrenIds.forEach(childId => { const currentChildMap = nodeMap.current[childId]; nodeMap.current[childId] = { ...currentChildMap, parent: id, id: childId }; }); From 90cd7c3c357e0664aa02b4f61b013470a012a1e9 Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Sun, 3 Nov 2019 14:56:07 +0000 Subject: [PATCH 6/6] Add test --- .../src/TreeView/TreeView.test.js | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/packages/material-ui-lab/src/TreeView/TreeView.test.js b/packages/material-ui-lab/src/TreeView/TreeView.test.js index e298f9c6b74735..12601505c05ad2 100644 --- a/packages/material-ui-lab/src/TreeView/TreeView.test.js +++ b/packages/material-ui-lab/src/TreeView/TreeView.test.js @@ -21,6 +21,38 @@ describe('', () => { after: () => mount.cleanUp(), })); + it('should not error when component state changes', () => { + function MyComponent() { + const [, setState] = React.useState(1); + + return ( + + { + setState(Math.random); + }} + > + + + + ); + } + + const { getByText, getByTestId } = render(); + + fireEvent.click(getByText('one')); + expect(getByTestId('one')).to.be.focused; + fireEvent.keyDown(document.activeElement, { key: 'ArrowDown' }); + expect(getByTestId('two')).to.be.focused; + fireEvent.keyDown(document.activeElement, { key: 'ArrowUp' }); + expect(getByTestId('one')).to.be.focused; + fireEvent.keyDown(document.activeElement, { key: 'ArrowDown' }); + expect(getByTestId('two')).to.be.focused; + }); + describe('onNodeToggle', () => { it('should be called when a parent node is clicked', () => { const handleNodeToggle = spy();