Tried to implement two setState pattern, but it does not feel right
Brian Vaughn committed
Apr 21, 2019 at 12:16 UTC
3de18de25ec3c8b28423ab53a760f4cc2087c95c
3 files changed
+84
-72
src/devtools/views/Components/InspectedElementContext.js
+10
-38
@@ -10,7 +10,6 @@ import React, {
10
import { createResource } from '../../cache';
11
import { BridgeContext, StoreContext } from '../context';
12
import { hydrate } from 'src/hydration';
13
-import { unstable_next as next } from 'scheduler';
13
import { TreeContext } from './TreeContext';
14
15
import type {
@@ -19,10 +18,7 @@ import type {
18
} from 'src/devtools/views/Components/types';
19
import type { Resource } from '../../cache';
20
22
-// TODO This isn't using the "two setState" pattern and updates sometimes feel janky.
23
-
21
type Context = {|
25
- inspectedElementID: number | null,
22
read(id: number): InspectedElement | null,
23
|};
24
@@ -42,40 +38,19 @@ type Props = {|
38
function InspectedElementContextController({ children }: Props) {
39
const bridge = useContext(BridgeContext);
40
const store = useContext(StoreContext);
41
+ const { inspectedElementID } = useContext(TreeContext);
42
46
- const { selectedElementID } = useContext(TreeContext);
47
- const [inspectedElement, setInspectedElement] = useState<{
48
- id: number | null,
49
- inspectedElement: InspectedElement | null,
50
- }>({
51
- id: selectedElementID,
52
- inspectedElement: null,
53
- });
54
- if (inspectedElement.id !== selectedElementID) {
55
- if (selectedElementID === null) {
56
- setInspectedElement({
57
- id: selectedElementID,
58
- inspectedElement: null,
59
- });
60
- } else {
61
- next(() =>
62
- setInspectedElement({
63
- id: selectedElementID,
64
- inspectedElement: null,
65
- })
66
- );
67
- }
68
- }
43
+ const [count, setCount] = useState<number>(0);
44
45
useEffect(() => {
71
- if (inspectedElement.id === null) {
46
+ if (inspectedElementID === null) {
47
return () => {};
48
}
49
75
- const rendererID = store.getRendererIDForElement(inspectedElement.id);
50
+ const rendererID = store.getRendererIDForElement(inspectedElementID);
51
52
const requestUpdate = () => {
78
- bridge.send('inspectElement', { id: inspectedElement.id, rendererID });
53
+ bridge.send('inspectElement', { id: inspectedElementID, rendererID });
54
};
55
56
requestUpdate();
@@ -83,7 +58,7 @@ function InspectedElementContextController({ children }: Props) {
58
const intervalID = setInterval(requestUpdate, 1000);
59
60
return () => clearInterval(intervalID);
86
- }, [bridge, inspectedElement.id, store]);
61
+ }, [bridge, inspectedElementID, store]);
62
63
const inProgressRequests = useMemo<Map<number, InProgressRequest>>(
64
() => new Map(),
@@ -136,9 +111,7 @@ function InspectedElementContextController({ children }: Props) {
111
resource.write(id, inspectedElement);
112
113
// Schedule update with React if necessary.
139
- setInspectedElement(prevState =>
140
- prevState.id === id ? { id, inspectedElement } : prevState
141
- );
114
+ setCount(count => count + 1);
115
}
116
}
117
};
@@ -147,14 +120,13 @@ function InspectedElementContextController({ children }: Props) {
120
return () => bridge.removeListener('inspectElement', onInspectedElement);
121
}, [bridge, inProgressRequests, resource]);
122
150
- // We intentionally use the broader inspectedElement object, rather than the id,
151
- // to enable updates to be scheduled with React after the cache has been invalidated.
123
const value = useMemo(
124
() => ({
154
- inspectedElementID: inspectedElement.id,
125
read: resource.read,
126
}),
157
- [inspectedElement, resource.read]
127
+ // Count is used to invalidate the cache and schedule an update with React.
128
+ // eslint-disable-next-line react-hooks/exhaustive-deps
129
+ [count, resource.read]
130
);
131
132
return (
src/devtools/views/Components/SelectedElement.js
+2
-2
@@ -22,11 +22,11 @@ import type { Element, InspectedElement } from './types';
22
export type Props = {||};
23
24
export default function SelectedElement(_: Props) {
25
- const { viewElementSource } = useContext(TreeContext);
25
+ const { inspectedElementID, viewElementSource } = useContext(TreeContext);
26
const bridge = useContext(BridgeContext);
27
const store = useContext(StoreContext);
28
29
- const { inspectedElementID, read } = useContext(InspectedElementContext);
29
+ const { read } = useContext(InspectedElementContext);
30
31
const element =
32
inspectedElementID !== null
src/devtools/views/Components/TreeContext.js
+72
-32
@@ -27,16 +27,13 @@ import React, {
27
useReducer,
28
useRef,
29
} from 'react';
30
+import { unstable_next as next } from 'scheduler';
31
import { createRegExp } from '../utils';
32
import { BridgeContext, StoreContext } from '../context';
33
import Store from '../../store';
34
35
import type { Element } from './types';
36
36
-// TODO Use two setState pattern for selecting Fibers:
37
-// The first update should be default priority and should select a new element in the Tree.
38
-// The second update should be deferred priority and should trigger suspense.
39
-
37
type Context = {|
38
// Tree
39
baseDepth: number,
@@ -67,6 +64,10 @@ type Context = {|
64
65
// Injected by parent HTML/JavaScript
66
viewElementSource: Function | null,
67
+
68
+ // Inspection element panel
69
+ // Updated separately so we can avoid suspending when selection changes
70
+ inspectedElementID: number | null,
71
|};
72
73
const TreeContext = createContext<Context>(((null: any): Context));
@@ -88,6 +89,9 @@ type State = {|
89
ownerStack: Array<number>,
90
ownerStackIndex: number | null,
91
_ownerFlatTree: Array<number> | null,
92
+
93
+ // Inspection element panel
94
+ inspectedElementID: number | null,
95
|};
96
97
type Action = {|
@@ -103,7 +107,8 @@ type Action = {|
107
| 'SELECT_PARENT_ELEMENT_IN_TREE'
108
| 'SELECT_PREVIOUS_ELEMENT_IN_TREE'
109
| 'SELECT_OWNER'
106
- | 'SET_SEARCH_TEXT',
110
+ | 'SET_SEARCH_TEXT'
111
+ | 'UPDATE_INSPECTED_ELEMENT_ID',
112
payload?: any,
113
|};
114
@@ -566,6 +571,24 @@ function reduceOwnersState(store: Store, state: State, action: Action): State {
571
};
572
}
573
574
+function reduceSuspenseState(
575
+ store: Store,
576
+ state: State,
577
+ action: Action
578
+): State {
579
+ const { type } = action;
580
+ switch (type) {
581
+ case 'UPDATE_INSPECTED_ELEMENT_ID':
582
+ return {
583
+ ...state,
584
+ inspectedElementID: state.selectedElementID,
585
+ };
586
+ default:
587
+ // React can bailout of no-op updates.
588
+ return state;
589
+ }
590
+}
591
+
592
type Props = {|
593
children: React$Node,
594
viewElementSource: Function | null,
@@ -596,10 +619,12 @@ function TreeContextController({ children, viewElementSource }: Props) {
619
case 'SELECT_PARENT_ELEMENT_IN_TREE':
620
case 'SELECT_PREVIOUS_ELEMENT_IN_TREE':
621
case 'SELECT_OWNER':
622
+ case 'UPDATE_INSPECTED_ELEMENT_ID':
623
case 'SET_SEARCH_TEXT':
624
state = reduceTreeState(store, state, action);
625
state = reduceSearchState(store, state, action);
626
state = reduceOwnersState(store, state, action);
627
+ state = reduceSuspenseState(store, state, action);
628
629
// If the selected ID is in a collapsed subtree, reset the selected index to null.
630
// We'll know the correct index after the layout effect will toggle the tree,
@@ -638,8 +663,19 @@ function TreeContextController({ children, viewElementSource }: Props) {
663
ownerStack: [],
664
ownerStackIndex: null,
665
_ownerFlatTree: null,
666
+
667
+ // Inspection element panel
668
+ inspectedElementID: null,
669
});
670
671
+ const dispatchWrapper = useCallback(
672
+ params => {
673
+ dispatch(params);
674
+ next(() => dispatch({ type: 'UPDATE_INSPECTED_ELEMENT_ID' }));
675
+ },
676
+ [dispatch]
677
+ );
678
+
679
const getElementAtIndex = useCallback(
680
(index: number) => {
681
return state._ownerFlatTree === null
@@ -650,49 +686,50 @@ function TreeContextController({ children, viewElementSource }: Props) {
686
);
687
const selectElementAtIndex = useCallback(
688
(index: number) =>
653
- dispatch({ type: 'SELECT_ELEMENT_AT_INDEX', payload: index }),
654
- [dispatch]
689
+ dispatchWrapper({ type: 'SELECT_ELEMENT_AT_INDEX', payload: index }),
690
+ [dispatchWrapper]
691
);
692
const selectElementByID = useCallback(
693
(id: number | null) =>
658
- dispatch({ type: 'SELECT_ELEMENT_BY_ID', payload: id }),
659
- [dispatch]
694
+ dispatchWrapper({ type: 'SELECT_ELEMENT_BY_ID', payload: id }),
695
+ [dispatchWrapper]
696
);
697
const setSearchText = useCallback(
662
- (text: string) => dispatch({ type: 'SET_SEARCH_TEXT', payload: text }),
663
- [dispatch]
698
+ (text: string) =>
699
+ dispatchWrapper({ type: 'SET_SEARCH_TEXT', payload: text }),
700
+ [dispatchWrapper]
701
);
702
const goToNextSearchResult = useCallback(
666
- () => dispatch({ type: 'GO_TO_NEXT_SEARCH_RESULT' }),
667
- [dispatch]
703
+ () => dispatchWrapper({ type: 'GO_TO_NEXT_SEARCH_RESULT' }),
704
+ [dispatchWrapper]
705
);
706
const goToPreviousSearchResult = useCallback(
670
- () => dispatch({ type: 'GO_TO_PREVIOUS_SEARCH_RESULT' }),
671
- [dispatch]
707
+ () => dispatchWrapper({ type: 'GO_TO_PREVIOUS_SEARCH_RESULT' }),
708
+ [dispatchWrapper]
709
);
710
const resetOwnerStack = useCallback(
674
- () => dispatch({ type: 'RESET_OWNER_STACK' }),
675
- [dispatch]
711
+ () => dispatchWrapper({ type: 'RESET_OWNER_STACK' }),
712
+ [dispatchWrapper]
713
);
714
const selectChildElementInTree = useCallback(
678
- () => dispatch({ type: 'SELECT_CHILD_ELEMENT_IN_TREE' }),
679
- [dispatch]
715
+ () => dispatchWrapper({ type: 'SELECT_CHILD_ELEMENT_IN_TREE' }),
716
+ [dispatchWrapper]
717
);
718
const selectNextElementInTree = useCallback(
682
- () => dispatch({ type: 'SELECT_NEXT_ELEMENT_IN_TREE' }),
683
- [dispatch]
719
+ () => dispatchWrapper({ type: 'SELECT_NEXT_ELEMENT_IN_TREE' }),
720
+ [dispatchWrapper]
721
);
722
const selectParentElementInTree = useCallback(
686
- () => dispatch({ type: 'SELECT_PARENT_ELEMENT_IN_TREE' }),
687
- [dispatch]
723
+ () => dispatchWrapper({ type: 'SELECT_PARENT_ELEMENT_IN_TREE' }),
724
+ [dispatchWrapper]
725
);
726
const selectPreviousElementInTree = useCallback(
690
- () => dispatch({ type: 'SELECT_PREVIOUS_ELEMENT_IN_TREE' }),
691
- [dispatch]
727
+ () => dispatchWrapper({ type: 'SELECT_PREVIOUS_ELEMENT_IN_TREE' }),
728
+ [dispatchWrapper]
729
);
730
const selectOwner = useCallback(
694
- (id: number) => dispatch({ type: 'SELECT_OWNER', payload: id }),
695
- [dispatch]
731
+ (id: number) => dispatchWrapper({ type: 'SELECT_OWNER', payload: id }),
732
+ [dispatchWrapper]
733
);
734
735
const value = useMemo(
@@ -724,6 +761,9 @@ function TreeContextController({ children, viewElementSource }: Props) {
761
resetOwnerStack,
762
selectOwner,
763
764
+ // Inspection element panel
765
+ inspectedElementID: state.inspectedElementID,
766
+
767
// Injected by parent HTML/JavaScript
768
viewElementSource,
769
}),
@@ -748,10 +788,10 @@ function TreeContextController({ children, viewElementSource }: Props) {
788
// Listen for host element selections.
789
useEffect(() => {
790
const handleSelectFiber = (id: number) =>
751
- dispatch({ type: 'SELECT_ELEMENT_BY_ID', payload: id });
791
+ dispatchWrapper({ type: 'SELECT_ELEMENT_BY_ID', payload: id });
792
bridge.addListener('selectFiber', handleSelectFiber);
793
return () => bridge.removeListener('selectFiber', handleSelectFiber);
754
- }, [bridge, dispatch]);
794
+ }, [bridge, dispatchWrapper]);
795
796
// If a newly-selected search result or inspection selection is inside of a collapsed subtree, auto expand it.
797
// This needs to be a layout effect to avoid temporarily flashing an incorrect selection.
@@ -775,7 +815,7 @@ function TreeContextController({ children, viewElementSource }: Props) {
815
addedElementIDs,
816
removedElementIDs,
817
]: Array<Uint32Array>) => {
778
- dispatch({
818
+ dispatchWrapper({
819
type: 'HANDLE_STORE_MUTATION',
820
payload: [addedElementIDs, removedElementIDs],
821
});
@@ -786,7 +826,7 @@ function TreeContextController({ children, viewElementSource }: Props) {
826
// At the moment, we can treat this as a mutation.
827
// We don't know which Elements were newly added/removed, but that should be okay in this case.
828
// It would only impact the search state, which is unlikely to exist yet at this point.
789
- dispatch({
829
+ dispatchWrapper({
830
type: 'HANDLE_STORE_MUTATION',
831
payload: [new Uint32Array(0), new Uint32Array(0)],
832
});
@@ -795,7 +835,7 @@ function TreeContextController({ children, viewElementSource }: Props) {
835
store.addListener('mutated', handleStoreMutated);
836
837
return () => store.removeListener('mutated', handleStoreMutated);
798
- }, [dispatch, initialRevision, store]);
838
+ }, [dispatchWrapper, initialRevision, store]);
839
840
return <TreeContext.Provider value={value}>{children}</TreeContext.Provider>;
841
}