Small refinements to Store: read-only Elements in map, added revision to guard against tearing

src/devtools/store.js

@@ -28,14 +28,18 @@ const debug = (methodName, ...args) => {
  * ContextProviders can subscribe to the Store for specific things they want to provide.
  */
 export default class Store extends EventEmitter {
-  // TODO Should items in this map be read-only/immutable for easier props comparison?
-  //      We currently mutate "children" and "weight" props.
+  // Map of ID to Element.
+  // Elements are read-only so they wokr well with memoization.
   _idToElement: Map<number, Element> = new Map();
 
   // Total number of visible elements (within all roots).
   // Used for windowing purposes.
   _numElements: number = 0;
 
+  // Incremented each time the store is mutated.
+  // This enables a passive effect to detect a mutation between render and commit phase.
+  _revision: number = 0;
+
   // This Array must be treated as immutable!
   // Passive effects will check it for changes between render and mount.
   _roots: $ReadOnlyArray<number> = [];
@@ -55,6 +59,10 @@ export default class Store extends EventEmitter {
     return this._numElements;
   }
 
+  get revision(): number {
+    return this._revision;
+  }
+
   get roots(): $ReadOnlyArray<number> {
     return this._roots;
   }
@@ -275,7 +283,11 @@ export default class Store extends EventEmitter {
               // Maybe in the future we'll revisit this.
             } else {
               parentElement = ((this._idToElement.get(parentID): any): Element);
-              parentElement.children = parentElement.children.concat(id);
+
+              this._idToElement.set(parentID, {
+                ...parentElement,
+                children: parentElement.children.concat(id),
+              });
 
               const element: Element = {
                 children: [],
@@ -319,9 +331,12 @@ export default class Store extends EventEmitter {
             this._roots = this._roots.filter(rootID => rootID !== id);
             this._rootIDToRendererID.delete(id);
           } else {
-            parentElement.children = parentElement.children.filter(
-              childID => childID !== id
-            );
+            this._idToElement.set(parentID, {
+              ...parentElement,
+              children: parentElement.children.filter(
+                childID => childID !== id
+              ),
+            });
           }
 
           // Track removed items so search results can be updated
@@ -343,7 +358,6 @@ export default class Store extends EventEmitter {
           debug('Re-order', `fiber ${id} children ${children.join(',')}`);
 
           element = ((this._idToElement.get(id): any): Element);
-          element.children = Array.from(children);
 
           const prevWeight = element.weight;
           let childWeight = 0;
@@ -353,7 +367,12 @@ export default class Store extends EventEmitter {
             childWeight += child.weight;
           });
 
-          element.weight = childWeight + 1;
+          this._idToElement.set(id, {
+            ...element,
+            children: Array.from(children),
+            weight: childWeight + 1,
+          });
+
           weightDelta = childWeight + 1 - prevWeight;
           break;
         default:
@@ -370,6 +389,8 @@ export default class Store extends EventEmitter {
       }
     }
 
+    this._revision++;
+
     if (haveRootsChanged) {
       this.emit('roots');
     }

src/devtools/types.js

@@ -18,7 +18,7 @@ export type ElementType = 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8;
 // Some of its information (e.g. id, type, displayName) come from the backend.
 // Other bits (e.g. weight and depth) are computed on the frontend for windowing and display purposes.
 // Elements are udpated on a push basis– meaning the backend pushes updates to the frontend when needed.
-export type Element = {|
+export type Element = $ReadOnly<{|
   id: number,
   parentID: number,
   children: Array<number>,
@@ -37,7 +37,7 @@ export type Element = {|
   // This property is used to quickly determine the total number of Elements,
   // and the Element at any given index (for windowing purposes).
   weight: number,
-|};
+|}>;
 
 export type Owner = {|
   displayName: string,

src/devtools/views/TreeContext.js

@@ -21,7 +21,7 @@ import React, {
   createContext,
   useCallback,
   useContext,
-  useLayoutEffect,
+  useEffect,
   useMemo,
   useReducer,
 } from 'react';
@@ -112,11 +112,11 @@ function reduceTreeState(store: Store, state: State, action: Action): State {
         numElements = store.numElements;
 
         // If the currently-selected Element has been removed from the tree, update selection state.
-        if (selectedElementID !== null) {
-          const removedElementIDs = ((payload: any): Array<Uint32Array>)[1];
-          if (removedElementIDs.includes(((selectedElementID: any): number))) {
-            selectedElementIndex = null;
-          }
+        if (
+          selectedElementID !== null &&
+          store.getElementByID(selectedElementID) === null
+        ) {
+          selectedElementIndex = null;
         }
         break;
       case 'SELECT_ELEMENT_AT_INDEX':
@@ -330,12 +330,9 @@ function reduceOwnersState(store: Store, state: State, action: Action): State {
   switch (type) {
     case 'HANDLE_STORE_MUTATION':
       if (ownerStack.length > 0) {
-        // eslint-disable-next-line no-unused-vars
-        const [_, removedElementIDs] = ((payload: any): Array<Uint32Array>);
-
         let indexOfRemovedItem = -1;
         for (let i = 0; i < ownerStack.length; i++) {
-          if (removedElementIDs.includes(ownerStack[i])) {
+          if (store.getElementByID(ownerStack[i]) === null) {
             indexOfRemovedItem = i;
             break;
           }
@@ -467,6 +464,7 @@ function reduceOwnersState(store: Store, state: State, action: Action): State {
 // TODO Remove TreeContextController wrapper element once global ConsearchText.write API exists.
 function TreeContextController({ children }: {| children: React$Node |}) {
   const store = useContext(StoreContext);
+  const initialRevision = useMemo(() => store.revision, [store]);
 
   // This reducer is created inline because it needs access to the Store.
   // The store is mutable, but the Store itself is global and lives for the lifetime of the DevTools,
@@ -592,7 +590,7 @@ function TreeContextController({ children }: {| children: React$Node |}) {
   );
 
   // Mutations to the underlying tree may impact this context (e.g. search results, selection state).
-  useLayoutEffect(() => {
+  useEffect(() => {
     const handleStoreMutated = ([
       addedElementIDs,
       removedElementIDs,
@@ -603,13 +601,21 @@ function TreeContextController({ children }: {| children: React$Node |}) {
       });
     };
 
-    // TODO Even though we're using layout effect, concurrent rendering may cause us to miss a mutation.
-    // Should the store expose some sort of version number that we could check after mounting?
+    // Since this is a passive effect, the tree may have been mutated before our initial subscription.
+    if (store.revision !== initialRevision) {
+      // At the moment, we can treat this as a mutation.
+      // We don't know which Elements were newly added/removed, but that should be okay in this case.
+      // It would only impact the search state, which is unlikely to exist yet at this point.
+      dispatch({
+        type: 'HANDLE_STORE_MUTATION',
+        payload: [new Uint32Array(0), new Uint32Array(0)],
+      });
+    }
 
     store.addListener('mutated', handleStoreMutated);
 
     return () => store.removeListener('mutated', handleStoreMutated);
-  }, [state, store]);
+  }, [store]);
 
   return <TreeContext.Provider value={value}>{children}</TreeContext.Provider>;
 }