diff --git a/src/devtools/store.js b/src/devtools/store.js index 4480a17ff9..42b3820e4d 100644 --- a/src/devtools/store.js +++ b/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 = 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 = []; @@ -55,6 +59,10 @@ export default class Store extends EventEmitter { return this._numElements; } + get revision(): number { + return this._revision; + } + get roots(): $ReadOnlyArray { 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'); } diff --git a/src/devtools/types.js b/src/devtools/types.js index 59cddbfd10..53517834dc 100644 --- a/src/devtools/types.js +++ b/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, @@ -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, diff --git a/src/devtools/views/TreeContext.js b/src/devtools/views/TreeContext.js index 2868fb728f..bc9232552c 100644 --- a/src/devtools/views/TreeContext.js +++ b/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)[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); - 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 {children}; }