From 564a2233685d52fceccfd2f4a515a654ec6f9879 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 9 May 2019 11:47:22 -0700 Subject: [PATCH 1/6] Fetch owners list from renderer (using suspense) Owners in the list may have been filtered out of the Store, but in the owners list view- it's important to still show them. The frontend cannot do this on its own, so this list needs to come from the renderer interface. --- src/backend/agent.js | 22 +- src/backend/renderer.js | 46 ++-- src/backend/types.js | 6 +- src/devtools/views/Components/Components.js | 29 +-- src/devtools/views/Components/Element.js | 4 +- .../views/Components/OwnersListContext.js | 109 +++++++++ src/devtools/views/Components/OwnersStack.css | 5 + src/devtools/views/Components/OwnersStack.js | 212 +++++++++++++----- .../views/Components/SelectedElement.js | 4 +- src/devtools/views/Components/Tree.css | 11 + src/devtools/views/Components/Tree.js | 20 +- src/devtools/views/Components/TreeContext.js | 113 +++------- src/devtools/views/Components/types.js | 5 + 13 files changed, 404 insertions(+), 182 deletions(-) create mode 100644 src/devtools/views/Components/OwnersListContext.js diff --git a/src/backend/agent.js b/src/backend/agent.js index 1a12eee29a..0c8fd77f09 100644 --- a/src/backend/agent.js +++ b/src/backend/agent.js @@ -16,6 +16,7 @@ import type { RendererID, RendererInterface, } from './types'; +import type { OwnersList } from 'src/devtools/views/Components/types'; import type { Bridge, ComponentFilter } from '../types'; const debug = (methodName, ...args) => { @@ -29,7 +30,7 @@ const debug = (methodName, ...args) => { } }; -type InspectSelectParams = {| +type ElementAndRendererID = {| id: number, rendererID: number, |}; @@ -99,6 +100,7 @@ export default class Agent extends EventEmitter { bridge.addListener('getProfilingStatus', this.getProfilingStatus); bridge.addListener('getProfilingSummary', this.getProfilingSummary); bridge.addListener('highlightElementInDOM', this.highlightElementInDOM); + bridge.addListener('getOwnersList', this.getOwnersList); bridge.addListener('inspectElement', this.inspectElement); bridge.addListener('logElementToConsole', this.logElementToConsole); bridge.addListener('overrideContext', this.overrideContext); @@ -303,7 +305,17 @@ export default class Agent extends EventEmitter { } }; - inspectElement = ({ id, rendererID }: InspectSelectParams) => { + getOwnersList = ({ id, rendererID }: ElementAndRendererID) => { + const renderer = this._rendererInterfaces[rendererID]; + if (renderer == null) { + console.warn(`Invalid renderer id "${rendererID}" for element "${id}"`); + } else { + const owners = renderer.getOwnersList(id); + this._bridge.send('ownersList', ({ id, owners }: OwnersList)); + } + }; + + inspectElement = ({ id, rendererID }: ElementAndRendererID) => { const renderer = this._rendererInterfaces[rendererID]; if (renderer == null) { console.warn(`Invalid renderer id "${rendererID}" for element "${id}"`); @@ -312,7 +324,7 @@ export default class Agent extends EventEmitter { } }; - logElementToConsole = ({ id, rendererID }: InspectSelectParams) => { + logElementToConsole = ({ id, rendererID }: ElementAndRendererID) => { const renderer = this._rendererInterfaces[rendererID]; if (renderer == null) { console.warn(`Invalid renderer id "${rendererID}" for element "${id}"`); @@ -340,7 +352,7 @@ export default class Agent extends EventEmitter { this._bridge.send('screenshotCaptured', { commitIndex, dataURL }); }; - selectElement = ({ id, rendererID }: InspectSelectParams) => { + selectElement = ({ id, rendererID }: ElementAndRendererID) => { const renderer = this._rendererInterfaces[rendererID]; if (renderer == null) { console.warn(`Invalid renderer id "${rendererID}" for element "${id}"`); @@ -506,7 +518,7 @@ export default class Agent extends EventEmitter { } }; - viewElementSource = ({ id, rendererID }: InspectSelectParams) => { + viewElementSource = ({ id, rendererID }: ElementAndRendererID) => { const renderer = this._rendererInterfaces[rendererID]; if (renderer == null) { console.warn(`Invalid renderer id "${rendererID}" for element "${id}"`); diff --git a/src/backend/renderer.js b/src/backend/renderer.js index c05c1dee05..cc49385c8f 100644 --- a/src/backend/renderer.js +++ b/src/backend/renderer.js @@ -50,7 +50,10 @@ import type { ReactRenderer, RendererInterface, } from './types'; -import type { InspectedElement } from 'src/devtools/views/Components/types'; +import type { + InspectedElement, + Owner, +} from 'src/devtools/views/Components/types'; import type { ComponentFilter, ElementType } from 'src/types'; function getInternalReactConstants(version) { @@ -1685,6 +1688,30 @@ export function attach( } } + function getOwnersList(id: number): Array | null { + let fiber = findCurrentFiberUsingSlowPathById(id); + if (fiber == null) { + return null; + } + + const { _debugOwner } = fiber; + + let owners = null; + if (_debugOwner) { + owners = []; + let owner = _debugOwner; + while (owner !== null) { + owners.push({ + displayName: getDisplayNameForFiber(owner) || 'Unknown', + id: getFiberID(getPrimaryFiber(owner)), + }); + owner = owner._debugOwner || null; + } + } + + return owners; + } + function inspectElementRaw(id: number): InspectedElement | null { let fiber = findCurrentFiberUsingSlowPathById(id); if (fiber == null) { @@ -1692,7 +1719,6 @@ export function attach( } const { - _debugOwner, _debugSource, stateNode, memoizedProps, @@ -1764,19 +1790,6 @@ export function attach( context = { value: context }; } - let owners = null; - if (_debugOwner) { - owners = []; - let owner = _debugOwner; - while (owner !== null) { - owners.push({ - displayName: getDisplayNameForFiber(owner) || 'Unknown', - id: getFiberID(getPrimaryFiber(owner)), - }); - owner = owner._debugOwner || null; - } - } - const isTimedOutSuspense = tag === SuspenseComponent && memoizedState !== null; @@ -1812,7 +1825,7 @@ export function attach( state: usesHooks ? null : memoizedState, // List of owners - owners, + owners: getOwnersList(id), // Location of component in source coude. source: _debugSource, @@ -2385,6 +2398,7 @@ export function attach( getFiberCommits, getInteractions, findNativeByFiberID, + getOwnersList, getPathForElement, getProfilingDataForDownload, getProfilingSummary, diff --git a/src/backend/types.js b/src/backend/types.js index 7cd529528b..a9680cd78a 100644 --- a/src/backend/types.js +++ b/src/backend/types.js @@ -1,7 +1,10 @@ // @flow import type { ComponentFilter, ElementType } from 'src/types'; -import type { InspectedElement } from 'src/devtools/views/Components/types'; +import type { + InspectedElement, + Owner, +} from 'src/devtools/views/Components/types'; type BundleType = | 0 // PROD @@ -175,6 +178,7 @@ export type RendererInterface = { ) => number | null, getFiberCommits: (rootID: number, fiberID: number) => FiberCommitsBackend, getInteractions: (rootID: number) => InteractionsBackend, + getOwnersList: (id: number) => Array | null, getProfilingDataForDownload: (rootID: number) => Object, getProfilingSummary: (rootID: number) => ProfilingSummaryBackend, getPathForElement: (id: number) => Array | null, diff --git a/src/devtools/views/Components/Components.js b/src/devtools/views/Components/Components.js index dd4b775431..3ed317a101 100644 --- a/src/devtools/views/Components/Components.js +++ b/src/devtools/views/Components/Components.js @@ -4,6 +4,7 @@ import React, { Suspense } from 'react'; import Tree from './Tree'; import SelectedElement from './SelectedElement'; import { InspectedElementContextController } from './InspectedElementContext'; +import { OwnersListContextController } from './OwnersListContext'; import portaledContent from '../portaledContent'; import { ModalDialog } from '../ModalDialog'; @@ -12,19 +13,21 @@ import styles from './Components.css'; function Components(_: {||}) { // TODO Flex wrappers below should be user resizable. return ( -
-
- -
-
- - }> - - - -
- -
+ + +
+
+ +
+
+ }> + + +
+ +
+ + ); } diff --git a/src/devtools/views/Components/Element.js b/src/devtools/views/Components/Element.js index 322726784d..79b015e85c 100644 --- a/src/devtools/views/Components/Element.js +++ b/src/devtools/views/Components/Element.js @@ -29,7 +29,7 @@ type Props = { export default function ElementView({ data, index, style }: Props) { const store = useContext(StoreContext); - const { ownerFlatTree, ownerStack, selectedElementID } = useContext( + const { ownerFlatTree, ownerID, selectedElementID } = useContext( TreeStateContext ); const dispatch = useContext(TreeDispatcherContext); @@ -168,7 +168,7 @@ export default function ElementView({ data, index, style }: Props) { }} > - {ownerStack.length === 0 ? ( + {ownerID === null ? ( ) : null} diff --git a/src/devtools/views/Components/OwnersListContext.js b/src/devtools/views/Components/OwnersListContext.js new file mode 100644 index 0000000000..3720f3b2cc --- /dev/null +++ b/src/devtools/views/Components/OwnersListContext.js @@ -0,0 +1,109 @@ +// @flow + +import React, { + createContext, + useCallback, + useContext, + useEffect, +} from 'react'; +import { createResource } from '../../cache'; +import { BridgeContext, StoreContext } from '../context'; +import { TreeStateContext } from './TreeContext'; + +import type { + Element, + Owner, + OwnersList, +} from 'src/devtools/views/Components/types'; +import type { Resource, Thenable } from '../../cache'; + +type Context = (id: number) => Array | null; + +const OwnersListContext = createContext(((null: any): Context)); +OwnersListContext.displayName = 'OwnersListContext'; + +type ResolveFn = (ownersList: Array | null) => void; +type InProgressRequest = {| + promise: Thenable>, + resolveFn: ResolveFn, +|}; + +const inProgressRequests: WeakMap = new WeakMap(); +const resource: Resource> = createResource( + (element: Element) => { + let request = inProgressRequests.get(element); + if (request != null) { + return request.promise; + } + + let resolveFn = ((null: any): ResolveFn); + const promise = new Promise(resolve => { + resolveFn = resolve; + }); + + inProgressRequests.set(element, { promise, resolveFn }); + + return promise; + }, + (element: Element) => element, + { useWeakMap: true } +); + +type Props = {| + children: React$Node, +|}; + +function OwnersListContextController({ children }: Props) { + const bridge = useContext(BridgeContext); + const store = useContext(StoreContext); + const { ownerID } = useContext(TreeStateContext); + + const read = useCallback( + (id: number) => { + const element = store.getElementByID(id); + if (element !== null) { + return resource.read(element); + } else { + return null; + } + }, + [store] + ); + + useEffect(() => { + const onOwnersList = (ownersList: OwnersList) => { + const id = ownersList.id; + + const element = store.getElementByID(id); + if (element !== null) { + const request = inProgressRequests.get(element); + if (request != null) { + inProgressRequests.delete(element); + request.resolveFn(ownersList.owners); + } + } + }; + + bridge.addListener('ownersList', onOwnersList); + return () => bridge.removeListener('ownersList', onOwnersList); + }, [bridge, store]); + + // This effect requests an updated owners list any time the selected owner changes + useEffect(() => { + if (ownerID !== null) { + const rendererID = store.getRendererIDForElement(ownerID); + + bridge.send('getOwnersList', { id: ownerID, rendererID }); + } + + return () => {}; + }, [bridge, ownerID, store]); + + return ( + + {children} + + ); +} + +export { OwnersListContext, OwnersListContextController }; diff --git a/src/devtools/views/Components/OwnersStack.css b/src/devtools/views/Components/OwnersStack.css index 1aba50714a..48c9db0fc4 100644 --- a/src/devtools/views/Components/OwnersStack.css +++ b/src/devtools/views/Components/OwnersStack.css @@ -91,3 +91,8 @@ font-family: var(--font-family-monospace); font-size: var(--font-size-monospace-normal); } + +.NotInStore, +.NotInStore:hover { + color: var(--color-dimmest); +} diff --git a/src/devtools/views/Components/OwnersStack.js b/src/devtools/views/Components/OwnersStack.js index 9635045d33..77ec8eed60 100644 --- a/src/devtools/views/Components/OwnersStack.js +++ b/src/devtools/views/Components/OwnersStack.js @@ -4,6 +4,7 @@ import React, { useCallback, useContext, useLayoutEffect, + useReducer, useRef, useState, } from 'react'; @@ -12,22 +13,111 @@ import { Menu, MenuList, MenuButton, MenuItem } from '@reach/menu-button'; import Button from '../Button'; import ButtonIcon from '../ButtonIcon'; import Toggle from '../Toggle'; +import { OwnersListContext } from './OwnersListContext'; import { TreeDispatcherContext, TreeStateContext } from './TreeContext'; -import { StoreContext } from '../context'; import { useIsOverflowing } from '../hooks'; +import { StoreContext } from '../context'; -import type { Element } from './types'; +import type { Owner } from './types'; import styles from './OwnersStack.css'; +type SelectOwner = (owner: Owner | null) => void; + +type ACTION_UPDATE_OWNER_ID = {| + type: 'UPDATE_OWNER_ID', + ownerID: number | null, + owners: Array, +|}; +type ACTION_UPDATE_SELECTED_INDEX = {| + type: 'UPDATE_SELECTED_INDEX', + selectedIndex: number, +|}; + +type Action = ACTION_UPDATE_OWNER_ID | ACTION_UPDATE_SELECTED_INDEX; + +type State = {| + ownerID: number | null, + owners: Array, + selectedIndex: number, +|}; + +function dialogReducer(state, action) { + switch (action.type) { + case 'UPDATE_OWNER_ID': + const selectedIndex = state.owners.findIndex( + owner => owner.id === action.ownerID + ); + return { + ownerID: action.ownerID, + owners: action.owners, + selectedIndex, + }; + case 'UPDATE_SELECTED_INDEX': + return { + ...state, + selectedIndex: action.selectedIndex, + }; + default: + throw new Error(`Invalid action "${action.type}"`); + } +} + export default function OwnerStack() { - const { ownerStack, ownerStackIndex } = useContext(TreeStateContext); - const dispatch = useContext(TreeDispatcherContext); + const read = useContext(OwnersListContext); + const { ownerID } = useContext(TreeStateContext); + const treeDispatch = useContext(TreeDispatcherContext); + + const [state, dispatch] = useReducer(dialogReducer, { + ownerID: null, + owners: [], + selectedIndex: -1, + }); + + // TODO (owners) Explain this and use reducer with ownerID too to avoid inf. loop + if (ownerID === null) { + dispatch({ + type: 'UPDATE_OWNER_ID', + ownerID: null, + owners: [], + }); + } else if (ownerID !== state.ownerID) { + const isInList = state.owners.findIndex(owner => owner.id === ownerID) >= 0; + dispatch({ + type: 'UPDATE_OWNER_ID', + ownerID, + owners: isInList ? state.owners : read(ownerID) || [], + }); + } + + const { owners, selectedIndex } = state; + + const selectOwner = useCallback( + (owner: Owner | null) => { + if (owner !== null) { + const index = owners.indexOf(owner); + dispatch({ + type: 'UPDATE_SELECTED_INDEX', + selectedIndex: index >= 0 ? index : 0, + }); + treeDispatch({ type: 'SELECT_OWNER', payload: owner.id }); + } else { + dispatch({ + type: 'UPDATE_SELECTED_INDEX', + selectedIndex: 0, + }); + treeDispatch({ type: 'RESET_OWNER_STACK' }); + } + }, + [owners, treeDispatch] + ); const [elementsTotalWidth, setElementsTotalWidth] = useState(0); const elementsBarRef = useRef(null); const isOverflowing = useIsOverflowing(elementsBarRef, elementsTotalWidth); + const selectedOwner = owners[selectedIndex]; + useLayoutEffect(() => { // If we're already overflowing, then we don't need to re-measure items. // That's because once the owners stack is open, it can only get larger (by driling in). @@ -37,7 +127,7 @@ export default function OwnerStack() { } let elementsTotalWidth = 0; - for (let i = 0; i < ownerStack.length; i++) { + for (let i = 0; i < owners.length; i++) { const element = elementsBarRef.current.children[i]; const computedStyle = getComputedStyle(element); @@ -48,7 +138,7 @@ export default function OwnerStack() { } setElementsTotalWidth(elementsTotalWidth); - }, [elementsBarRef, isOverflowing, ownerStack.length]); + }, [elementsBarRef, isOverflowing, owners.length]); return (
@@ -56,28 +146,38 @@ export default function OwnerStack() { {isOverflowing && ( - + {selectedOwner != null && ( + + )} )} {!isOverflowing && - ownerStack.map((id, index) => ( - + owners.map((owner, index) => ( + ))}
diff --git a/src/devtools/views/Components/SelectedElement.js b/src/devtools/views/Components/SelectedElement.js index 90da16ee48..4ead00425a 100644 --- a/src/devtools/views/Components/SelectedElement.js +++ b/src/devtools/views/Components/SelectedElement.js @@ -227,7 +227,7 @@ function InspectedElementView({ state, } = inspectedElement; - const { ownerStack } = useContext(TreeStateContext); + const { ownerID } = useContext(TreeStateContext); const bridge = useContext(BridgeContext); const store = useContext(StoreContext); @@ -298,7 +298,7 @@ function InspectedElementView({ overrideValueFn={overrideContextFn} /> - {ownerStack.length === 0 && owners !== null && owners.length > 0 && ( + {ownerID === null && owners !== null && owners.length > 0 && (
rendered by
{owners.map(owner => ( diff --git a/src/devtools/views/Components/Tree.css b/src/devtools/views/Components/Tree.css index 3ce44f0828..3efc1bf863 100644 --- a/src/devtools/views/Components/Tree.css +++ b/src/devtools/views/Components/Tree.css @@ -37,3 +37,14 @@ margin: 0 0.5rem; background-color: var(--color-border); } + +.Loading { + height: 100%; + padding-left: 0.5rem; + display: flex; + align-items: center; + flex: 1; + justify-content: flex-start; + font-size: var(--font-size-sans-large); + color: var(--color-dim); +} diff --git a/src/devtools/views/Components/Tree.js b/src/devtools/views/Components/Tree.js index 8c3c4031f7..4687c79939 100644 --- a/src/devtools/views/Components/Tree.js +++ b/src/devtools/views/Components/Tree.js @@ -1,6 +1,7 @@ // @flow import React, { + Suspense, useState, useCallback, useContext, @@ -38,7 +39,7 @@ export default function Tree(props: Props) { const dispatch = useContext(TreeDispatcherContext); const { numElements, - ownerStack, + ownerID, searchIndex, searchResults, selectedElementID, @@ -277,7 +278,9 @@ export default function Tree(props: Props) {
- {ownerStack.length > 0 ? : } + }> + {ownerID !== null ? : } +
@@ -317,7 +320,7 @@ export default function Tree(props: Props) { } function InnerElementType({ style, ...rest }) { - const { ownerStack } = useContext(TreeStateContext); + const { ownerID } = useContext(TreeStateContext); // The list may need to scroll horizontally due to deeply nested elements. // We don't know the maximum scroll width up front, because we're windowing. @@ -346,10 +349,9 @@ function InnerElementType({ style, ...rest }) { // We shouldn't retain this width across different conceptual trees though, // so when the user opens the "owners tree" view, we should discard the previous width. - const hasOwnerStack = ownerStack.length > 0; - const [prevHasOwnerStack, setPrevHasOwnerStack] = useState(hasOwnerStack); - if (hasOwnerStack !== prevHasOwnerStack) { - setPrevHasOwnerStack(hasOwnerStack); + const [prevOwnerID, setPrevOwnerID] = useState(ownerID); + if (ownerID !== prevOwnerID) { + setPrevOwnerID(ownerID); setMinWidth(null); } @@ -371,3 +373,7 @@ function InnerElementType({ style, ...rest }) { /> ); } + +function Loading() { + return
Loading...
; +} diff --git a/src/devtools/views/Components/TreeContext.js b/src/devtools/views/Components/TreeContext.js index 002f6b619e..b2c9b29ac4 100644 --- a/src/devtools/views/Components/TreeContext.js +++ b/src/devtools/views/Components/TreeContext.js @@ -50,9 +50,8 @@ type StateContext = {| searchText: string, // Owners + ownerID: number | null, ownerFlatTree: Array | null, - ownerStack: Array, - ownerStackIndex: number | null, // Inspection element panel inspectedElementID: number | null, @@ -142,8 +141,7 @@ type State = {| searchText: string, // Owners - ownerStack: Array, - ownerStackIndex: number | null, + ownerID: number | null, ownerFlatTree: Array | null, // Inspection element panel @@ -151,17 +149,12 @@ type State = {| |}; function reduceTreeState(store: Store, state: State, action: Action): State { - let { - numElements, - ownerStack, - selectedElementIndex, - selectedElementID, - } = state; + let { numElements, ownerID, selectedElementIndex, selectedElementID } = state; let lookupIDForIndex = true; // Base tree should ignore selected element changes when the owner's tree is active. - if (ownerStack.length === 0) { + if (ownerID === null) { switch (action.type) { case 'HANDLE_STORE_MUTATION': numElements = store.numElements; @@ -276,7 +269,7 @@ function reduceTreeState(store: Store, state: State, action: Action): State { function reduceSearchState(store: Store, state: State, action: Action): State { let { - ownerStack, + ownerID, searchIndex, searchResults, searchText, @@ -295,7 +288,7 @@ function reduceSearchState(store: Store, state: State, action: Action): State { let didRequestSearch = false; // Search isn't supported when the owner's tree is active. - if (ownerStack.length === 0) { + if (ownerID === null) { switch (action.type) { case 'GO_TO_NEXT_SEARCH_RESULT': if (numPrevSearchResults > 0) { @@ -442,9 +435,8 @@ function reduceOwnersState(store: Store, state: State, action: Action): State { numElements, selectedElementID, selectedElementIndex, + ownerID, ownerFlatTree, - ownerStack, - ownerStackIndex, searchIndex, searchResults, searchText, @@ -454,25 +446,9 @@ function reduceOwnersState(store: Store, state: State, action: Action): State { switch (action.type) { case 'HANDLE_STORE_MUTATION': - if (ownerStack.length > 0) { - let indexOfRemovedItem = -1; - for (let i = 0; i < ownerStack.length; i++) { - if (store.getElementByID(ownerStack[i]) === null) { - indexOfRemovedItem = i; - break; - } - } - - if (indexOfRemovedItem >= 0) { - ownerStack = ownerStack.slice(0, indexOfRemovedItem); - if (ownerStack.length === 0) { - ownerFlatTree = null; - ownerStackIndex = null; - } else { - ownerStackIndex = ownerStack.length - 1; - } - } - if (selectedElementID !== null && ownerFlatTree !== null) { + if (ownerID !== null) { + ownerFlatTree = store.getOwnersListForElement(ownerID); + if (selectedElementID !== null) { // Mutation might have caused the index of this ID to shift. selectedElementIndex = ownerFlatTree.findIndex( element => element.id === selectedElementID @@ -491,13 +467,12 @@ function reduceOwnersState(store: Store, state: State, action: Action): State { } break; case 'RESET_OWNER_STACK': - ownerStack = []; - ownerStackIndex = null; + ownerID = null; + ownerFlatTree = null; selectedElementIndex = selectedElementID !== null ? store.getIndexOfElementID(selectedElementID) : null; - ownerFlatTree = null; break; case 'SELECT_ELEMENT_AT_INDEX': if (ownerFlatTree !== null) { @@ -533,33 +508,12 @@ function reduceOwnersState(store: Store, state: State, action: Action): State { // If the Store doesn't have any owners metadata, don't drill into an empty stack. // This is a confusing user experience. if (store.hasOwnerMetadata) { - const id = (action: ACTION_SELECT_OWNER).payload; - ownerStackIndex = ownerStack.indexOf(id); + ownerID = (action: ACTION_SELECT_OWNER).payload; + ownerFlatTree = store.getOwnersListForElement(ownerID); // Always force reset selection to be the top of the new owner tree. selectedElementIndex = 0; prevSelectedElementIndex = null; - - // If this owner is already in the current stack, just select it. - // Otherwise, create a new stack. - if (ownerStackIndex < 0) { - // Add this new owner, and fill in the owners above it as well. - ownerStack = []; - let currentOwnerID = id; - while (currentOwnerID !== 0) { - ownerStack.unshift(currentOwnerID); - currentOwnerID = ((store.getElementByID( - currentOwnerID - ): any): Element).ownerID; - } - ownerStackIndex = ownerStack.length - 1; - - if (searchText !== '') { - searchIndex = null; - searchResults = []; - searchText = ''; - } - } } break; default: @@ -569,17 +523,12 @@ function reduceOwnersState(store: Store, state: State, action: Action): State { // Changes in the selected owner require re-calculating the owners tree. if ( - ownerStackIndex !== state.ownerStackIndex || - ownerStack !== state.ownerStack || + ownerFlatTree !== state.ownerFlatTree || action.type === 'HANDLE_STORE_MUTATION' ) { - if (ownerStackIndex === null) { - ownerFlatTree = null; + if (ownerFlatTree === null) { numElements = store.numElements; } else { - ownerFlatTree = store.getOwnersListForElement( - ownerStack[ownerStackIndex] - ); numElements = ownerFlatTree.length; } } @@ -588,9 +537,10 @@ function reduceOwnersState(store: Store, state: State, action: Action): State { if (selectedElementIndex !== prevSelectedElementIndex) { if (selectedElementIndex === null) { selectedElementID = null; - } else if (ownerFlatTree !== null) { - selectedElementID = - ownerFlatTree[((selectedElementIndex: any): number)].id; + } else { + if (ownerFlatTree !== null) { + selectedElementID = ownerFlatTree[selectedElementIndex].id; + } } } @@ -605,8 +555,7 @@ function reduceOwnersState(store: Store, state: State, action: Action): State { searchResults, searchText, - ownerStack, - ownerStackIndex, + ownerID, ownerFlatTree, }; } @@ -619,14 +568,19 @@ function reduceSuspenseState( const { type } = action; switch (type) { case 'UPDATE_INSPECTED_ELEMENT_ID': - return { - ...state, - inspectedElementID: state.selectedElementID, - }; + if (state.inspectedElementID !== state.selectedElementID) { + return { + ...state, + inspectedElementID: state.selectedElementID, + }; + } + break; default: - // React can bailout of no-op updates. - return state; + break; } + + // React can bailout of no-op updates. + return state; } type Props = {| children: React$Node |}; @@ -696,8 +650,7 @@ function TreeContextController({ children }: Props) { searchText: '', // Owners - ownerStack: [], - ownerStackIndex: null, + ownerID: null, ownerFlatTree: null, // Inspection element panel diff --git a/src/devtools/views/Components/types.js b/src/devtools/views/Components/types.js index 0849ec019a..0e33349a47 100644 --- a/src/devtools/views/Components/types.js +++ b/src/devtools/views/Components/types.js @@ -35,6 +35,11 @@ export type Owner = {| id: number, |}; +export type OwnersList = {| + id: number, + owners: Array | null, +|}; + export type InspectedElement = {| id: number, From b6c135c165de5023ce09abb28c35458b41927789 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 9 May 2019 14:21:22 -0700 Subject: [PATCH 2/6] Added Jest tests for OwnersListContext --- .../ownersListContext-test.js.snap | 51 ++++++ src/__tests__/ownersListContext-test.js | 166 ++++++++++++++++++ src/__tests__/storeComponentFilters-test.js | 66 ++----- src/__tests__/utils.js | 52 ++++++ src/devtools/views/Components/TreeContext.js | 11 +- 5 files changed, 295 insertions(+), 51 deletions(-) create mode 100644 src/__tests__/__snapshots__/ownersListContext-test.js.snap create mode 100644 src/__tests__/ownersListContext-test.js diff --git a/src/__tests__/__snapshots__/ownersListContext-test.js.snap b/src/__tests__/__snapshots__/ownersListContext-test.js.snap new file mode 100644 index 0000000000..b4f05affaa --- /dev/null +++ b/src/__tests__/__snapshots__/ownersListContext-test.js.snap @@ -0,0 +1,51 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`profiling should fetch the owners list for the selected element that includes filtered components: mount 1`] = ` +[root] + ▾ + + +`; + +exports[`profiling should fetch the owners list for the selected element that includes filtered components: owners for "Child" 1`] = ` +Array [ + Object { + "displayName": "Parent", + "id": 9, + }, + Object { + "displayName": "Grandparent", + "id": 7, + }, +] +`; + +exports[`profiling should fetch the owners list for the selected element: mount 1`] = ` +[root] + ▾ + ▾ + + +`; + +exports[`profiling should fetch the owners list for the selected element: owners for "Child" 1`] = ` +Array [ + Object { + "displayName": "Parent", + "id": 3, + }, + Object { + "displayName": "Grandparent", + "id": 2, + }, +] +`; + +exports[`profiling should fetch the owners list for the selected element: owners for "Parent" 1`] = ` +Array [ + Object { + "displayName": "Grandparent", + "id": 2, + }, +] +`; diff --git a/src/__tests__/ownersListContext-test.js b/src/__tests__/ownersListContext-test.js new file mode 100644 index 0000000000..ebcf7e553c --- /dev/null +++ b/src/__tests__/ownersListContext-test.js @@ -0,0 +1,166 @@ +// @flow + +import typeof ReactTestRenderer from 'react-test-renderer'; +import type { Element } from 'src/devtools/views/Components/types'; +import type Bridge from 'src/bridge'; +import type Store from 'src/devtools/store'; + +describe('profiling', () => { + let React; + let ReactDOM; + let TestRenderer: ReactTestRenderer; + let bridge: Bridge; + let store: Store; + let utils; + + let BridgeContext; + let OwnersListContext; + let OwnersListContextController; + let StoreContext; + let TreeContextController; + + beforeEach(() => { + utils = require('./utils'); + utils.beforeEachProfiling(); + + bridge = global.bridge; + store = global.store; + store.collapseNodesByDefault = false; + + React = require('react'); + ReactDOM = require('react-dom'); + TestRenderer = utils.requireTestRenderer(); + + BridgeContext = require('src/devtools/views/context').BridgeContext; + OwnersListContext = require('src/devtools/views/Components/OwnersListContext') + .OwnersListContext; + OwnersListContextController = require('src/devtools/views/Components/OwnersListContext') + .OwnersListContextController; + StoreContext = require('src/devtools/views/context').StoreContext; + TreeContextController = require('src/devtools/views/Components/TreeContext') + .TreeContextController; + }); + + const Contexts = ({ children, defaultOwnerID = null }) => ( + + + + {children} + + + + ); + + it('should fetch the owners list for the selected element', async done => { + const Grandparent = () => ; + const Parent = ({ count }) => { + return ( + + + + + ); + }; + const Child = ({ duration }) => null; + + utils.act(() => + ReactDOM.render(, document.createElement('div')) + ); + + expect(store).toMatchSnapshot('mount'); + + const parent = ((store.getElementAtIndex(1): any): Element); + const firstChild = ((store.getElementAtIndex(2): any): Element); + + let didFinish = false; + + function Suspender({ owner }) { + const read = React.useContext(OwnersListContext); + const owners = read(owner.id); + expect(owners).toMatchSnapshot( + `owners for "${(owner && owner.displayName) || ''}"` + ); + didFinish = true; + return null; + } + + await utils.actSuspense( + () => + TestRenderer.create( + + + + + + ), + 3 + ); + expect(didFinish).toBe(true); + + didFinish = false; + await utils.actSuspense( + () => + TestRenderer.create( + + + + + + ), + 3 + ); + expect(didFinish).toBe(true); + + done(); + }); + + it('should fetch the owners list for the selected element that includes filtered components', async done => { + store.componentFilters = [utils.createDisplayNameFilter('^Parent$')]; + + const Grandparent = () => ; + const Parent = ({ count }) => { + return ( + + + + + ); + }; + const Child = ({ duration }) => null; + + utils.act(() => + ReactDOM.render(, document.createElement('div')) + ); + + expect(store).toMatchSnapshot('mount'); + + const firstChild = ((store.getElementAtIndex(1): any): Element); + + let didFinish = false; + + function Suspender({ owner }) { + const read = React.useContext(OwnersListContext); + const owners = read(owner.id); + expect(owners).toMatchSnapshot( + `owners for "${(owner && owner.displayName) || ''}"` + ); + didFinish = true; + return null; + } + + await utils.actSuspense( + () => + TestRenderer.create( + + + + + + ), + 3 + ); + expect(didFinish).toBe(true); + + done(); + }); +}); diff --git a/src/__tests__/storeComponentFilters-test.js b/src/__tests__/storeComponentFilters-test.js index 8f408f49f7..3030646bf6 100644 --- a/src/__tests__/storeComponentFilters-test.js +++ b/src/__tests__/storeComponentFilters-test.js @@ -6,42 +6,7 @@ describe('Store component filters', () => { let TestUtils; let Types; let store; - - const createElementTypeFilter = (elementType, isEnabled = true) => ({ - type: Types.ComponentFilterElementType, - isEnabled, - value: elementType, - }); - - const createDisplayNameFilter = (source, isEnabled = true) => { - let isValid = true; - try { - new RegExp(source); - } catch (error) { - isValid = false; - } - return { - type: Types.ComponentFilterDisplayName, - isEnabled, - isValid, - value: source, - }; - }; - - const createLocationFilter = (source, isEnabled = true) => { - let isValid = true; - try { - new RegExp(source); - } catch (error) { - isValid = false; - } - return { - type: Types.ComponentFilterLocation, - isEnabled, - isValid, - value: source, - }; - }; + let utils; const act = (callback: Function) => { TestUtils.act(() => { @@ -59,6 +24,7 @@ describe('Store component filters', () => { ReactDOM = require('react-dom'); TestUtils = require('react-dom/test-utils'); Types = require('src/types'); + utils = require('./utils'); }); it('should throw if filters are updated while profiling', () => { @@ -89,7 +55,7 @@ describe('Store component filters', () => { act( () => (store.componentFilters = [ - createElementTypeFilter(Types.ElementTypeHostComponent), + utils.createElementTypeFilter(Types.ElementTypeHostComponent), ]) ); @@ -98,7 +64,7 @@ describe('Store component filters', () => { act( () => (store.componentFilters = [ - createElementTypeFilter(Types.ElementTypeClass), + utils.createElementTypeFilter(Types.ElementTypeClass), ]) ); @@ -107,8 +73,8 @@ describe('Store component filters', () => { act( () => (store.componentFilters = [ - createElementTypeFilter(Types.ElementTypeClass), - createElementTypeFilter(Types.ElementTypeFunction), + utils.createElementTypeFilter(Types.ElementTypeClass), + utils.createElementTypeFilter(Types.ElementTypeFunction), ]) ); @@ -117,8 +83,8 @@ describe('Store component filters', () => { act( () => (store.componentFilters = [ - createElementTypeFilter(Types.ElementTypeClass, false), - createElementTypeFilter(Types.ElementTypeFunction, false), + utils.createElementTypeFilter(Types.ElementTypeClass, false), + utils.createElementTypeFilter(Types.ElementTypeFunction, false), ]) ); @@ -134,7 +100,7 @@ describe('Store component filters', () => { act( () => (store.componentFilters = [ - createElementTypeFilter(Types.ElementTypeRoot), + utils.createElementTypeFilter(Types.ElementTypeRoot), ]) ); @@ -159,13 +125,17 @@ describe('Store component filters', () => { ); expect(store).toMatchSnapshot('1: mount'); - act(() => (store.componentFilters = [createDisplayNameFilter('Foo')])); + act( + () => (store.componentFilters = [utils.createDisplayNameFilter('Foo')]) + ); expect(store).toMatchSnapshot('2: filter "Foo"'); - act(() => (store.componentFilters = [createDisplayNameFilter('Ba')])); + act(() => (store.componentFilters = [utils.createDisplayNameFilter('Ba')])); expect(store).toMatchSnapshot('3: filter "Ba"'); - act(() => (store.componentFilters = [createDisplayNameFilter('B.z')])); + act( + () => (store.componentFilters = [utils.createDisplayNameFilter('B.z')]) + ); expect(store).toMatchSnapshot('4: filter "B.z"'); }); @@ -178,7 +148,7 @@ describe('Store component filters', () => { act( () => (store.componentFilters = [ - createLocationFilter(__filename.replace(__dirname, '')), + utils.createLocationFilter(__filename.replace(__dirname, '')), ]) ); @@ -189,7 +159,7 @@ describe('Store component filters', () => { act( () => (store.componentFilters = [ - createLocationFilter('this:is:a:made:up:path'), + utils.createLocationFilter('this:is:a:made:up:path'), ]) ); diff --git a/src/__tests__/utils.js b/src/__tests__/utils.js index 89c7ffd6b1..06d8ec8858 100644 --- a/src/__tests__/utils.js +++ b/src/__tests__/utils.js @@ -2,6 +2,8 @@ import typeof ReactTestRenderer from 'react-test-renderer'; +import type { ElementType } from 'src/types'; + export function act(callback: Function): void { const TestUtils = require('react-dom/test-utils'); TestUtils.act(() => { @@ -54,6 +56,56 @@ export function beforeEachProfiling(): void { ); } +export function createElementTypeFilter( + elementType: ElementType, + isEnabled: boolean = true +) { + const Types = require('src/types'); + return { + type: Types.ComponentFilterElementType, + isEnabled, + value: elementType, + }; +} + +export function createDisplayNameFilter( + source: string, + isEnabled: boolean = true +) { + const Types = require('src/types'); + let isValid = true; + try { + new RegExp(source); + } catch (error) { + isValid = false; + } + return { + type: Types.ComponentFilterDisplayName, + isEnabled, + isValid, + value: source, + }; +} + +export function createLocationFilter( + source: string, + isEnabled: boolean = true +) { + const Types = require('src/types'); + let isValid = true; + try { + new RegExp(source); + } catch (error) { + isValid = false; + } + return { + type: Types.ComponentFilterLocation, + isEnabled, + isValid, + value: source, + }; +} + export function getRendererID(): number { if (global.agent == null) { throw Error('Agent unavailable.'); diff --git a/src/devtools/views/Components/TreeContext.js b/src/devtools/views/Components/TreeContext.js index b2c9b29ac4..6f65e7920d 100644 --- a/src/devtools/views/Components/TreeContext.js +++ b/src/devtools/views/Components/TreeContext.js @@ -583,10 +583,15 @@ function reduceSuspenseState( return state; } -type Props = {| children: React$Node |}; +type Props = {| + children: React$Node, + + // Used for automated testing + defaultOwnerID?: ?number, +|}; // TODO Remove TreeContextController wrapper element once global ConsearchText.write API exists. -function TreeContextController({ children }: Props) { +function TreeContextController({ children, defaultOwnerID }: Props) { const bridge = useContext(BridgeContext); const store = useContext(StoreContext); @@ -650,7 +655,7 @@ function TreeContextController({ children }: Props) { searchText: '', // Owners - ownerID: null, + ownerID: defaultOwnerID == null ? null : defaultOwnerID, ownerFlatTree: null, // Inspection element panel From 5daf9b626f12c291c3c46918ae0a01a8a94c7461 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 9 May 2019 14:31:48 -0700 Subject: [PATCH 3/6] Fixed test describe name --- .../__snapshots__/ownersListContext-test.js.snap | 10 +++++----- src/__tests__/ownersListContext-test.js | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/__tests__/__snapshots__/ownersListContext-test.js.snap b/src/__tests__/__snapshots__/ownersListContext-test.js.snap index b4f05affaa..74c908afe8 100644 --- a/src/__tests__/__snapshots__/ownersListContext-test.js.snap +++ b/src/__tests__/__snapshots__/ownersListContext-test.js.snap @@ -1,13 +1,13 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`profiling should fetch the owners list for the selected element that includes filtered components: mount 1`] = ` +exports[`OwnersListContext should fetch the owners list for the selected element that includes filtered components: mount 1`] = ` [root] ▾ `; -exports[`profiling should fetch the owners list for the selected element that includes filtered components: owners for "Child" 1`] = ` +exports[`OwnersListContext should fetch the owners list for the selected element that includes filtered components: owners for "Child" 1`] = ` Array [ Object { "displayName": "Parent", @@ -20,7 +20,7 @@ Array [ ] `; -exports[`profiling should fetch the owners list for the selected element: mount 1`] = ` +exports[`OwnersListContext should fetch the owners list for the selected element: mount 1`] = ` [root] ▾ ▾ @@ -28,7 +28,7 @@ exports[`profiling should fetch the owners list for the selected element: mount `; -exports[`profiling should fetch the owners list for the selected element: owners for "Child" 1`] = ` +exports[`OwnersListContext should fetch the owners list for the selected element: owners for "Child" 1`] = ` Array [ Object { "displayName": "Parent", @@ -41,7 +41,7 @@ Array [ ] `; -exports[`profiling should fetch the owners list for the selected element: owners for "Parent" 1`] = ` +exports[`OwnersListContext should fetch the owners list for the selected element: owners for "Parent" 1`] = ` Array [ Object { "displayName": "Grandparent", diff --git a/src/__tests__/ownersListContext-test.js b/src/__tests__/ownersListContext-test.js index ebcf7e553c..348f6932c4 100644 --- a/src/__tests__/ownersListContext-test.js +++ b/src/__tests__/ownersListContext-test.js @@ -5,7 +5,7 @@ import type { Element } from 'src/devtools/views/Components/types'; import type Bridge from 'src/bridge'; import type Store from 'src/devtools/store'; -describe('profiling', () => { +describe('OwnersListContext', () => { let React; let ReactDOM; let TestRenderer: ReactTestRenderer; From 427f0f63c1eeba330dc80b6fb3ac2930bc17e7fd Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 9 May 2019 15:52:50 -0700 Subject: [PATCH 4/6] Added TreeContext tests --- .../__snapshots__/treeContext-test.js.snap | 713 ++++++++++++++++++ src/__tests__/ownersListContext-test.js | 16 +- src/__tests__/treeContext-test.js | 428 +++++++++++ src/devtools/views/Components/TreeContext.js | 4 +- 4 files changed, 1151 insertions(+), 10 deletions(-) create mode 100644 src/__tests__/__snapshots__/treeContext-test.js.snap create mode 100644 src/__tests__/treeContext-test.js diff --git a/src/__tests__/__snapshots__/treeContext-test.js.snap b/src/__tests__/__snapshots__/treeContext-test.js.snap new file mode 100644 index 0000000000..fd9c4dbf1f --- /dev/null +++ b/src/__tests__/__snapshots__/treeContext-test.js.snap @@ -0,0 +1,713 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`TreeListContext owners state should support entering and existing the owners tree view: 0: mount 1`] = ` +[root] + ▾ + ▾ + + +`; + +exports[`TreeListContext owners state should support entering and existing the owners tree view: 1: initial state 1`] = ` +Object { + "inspectedElementID": null, + "numElements": 4, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": null, + "selectedElementIndex": null, +} +`; + +exports[`TreeListContext owners state should support entering and existing the owners tree view: 2: parent owners tree 1`] = ` +Object { + "inspectedElementID": 3, + "numElements": 3, + "ownerFlatTree": Array [ + Object { + "children": Array [ + 4, + 5, + ], + "depth": 0, + "displayName": "Parent", + "id": 3, + "isCollapsed": false, + "key": null, + "ownerID": 2, + "parentID": 2, + "type": 5, + "weight": 3, + }, + Object { + "children": Array [], + "depth": 1, + "displayName": "Child", + "id": 4, + "isCollapsed": false, + "key": null, + "ownerID": 3, + "parentID": 3, + "type": 5, + "weight": 1, + }, + Object { + "children": Array [], + "depth": 1, + "displayName": "Child", + "id": 5, + "isCollapsed": false, + "key": null, + "ownerID": 3, + "parentID": 3, + "type": 5, + "weight": 1, + }, + ], + "ownerID": 3, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": 3, + "selectedElementIndex": 0, +} +`; + +exports[`TreeListContext owners state should support entering and existing the owners tree view: 3: final state 1`] = ` +Object { + "inspectedElementID": 3, + "numElements": 4, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": 3, + "selectedElementIndex": 1, +} +`; + +exports[`TreeListContext search state should add newly mounted elements to the search results set if they match the current text: 0: mount 1`] = ` +[root] + + +`; + +exports[`TreeListContext search state should add newly mounted elements to the search results set if they match the current text: 1: initial state 1`] = ` +Object { + "inspectedElementID": null, + "numElements": 2, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": null, + "selectedElementIndex": null, +} +`; + +exports[`TreeListContext search state should add newly mounted elements to the search results set if they match the current text: 2: search for "ba" 1`] = ` +Object { + "inspectedElementID": 3, + "numElements": 2, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": 0, + "searchResults": Array [ + 3, + ], + "searchText": "ba", + "selectedElementID": 3, + "selectedElementIndex": 1, +} +`; + +exports[`TreeListContext search state should add newly mounted elements to the search results set if they match the current text: 3: mount Baz 1`] = ` +Object { + "inspectedElementID": 3, + "numElements": 3, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": 0, + "searchResults": Array [ + 3, + 4, + ], + "searchText": "ba", + "selectedElementID": 3, + "selectedElementIndex": 1, +} +`; + +exports[`TreeListContext search state should find elements matching search text: 0: mount 1`] = ` +[root] + + + +`; + +exports[`TreeListContext search state should find elements matching search text: 1: initial state 1`] = ` +Object { + "inspectedElementID": null, + "numElements": 3, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": null, + "selectedElementIndex": null, +} +`; + +exports[`TreeListContext search state should find elements matching search text: 2: search for "ba" 1`] = ` +Object { + "inspectedElementID": 3, + "numElements": 3, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": 0, + "searchResults": Array [ + 3, + 4, + ], + "searchText": "ba", + "selectedElementID": 3, + "selectedElementIndex": 1, +} +`; + +exports[`TreeListContext search state should find elements matching search text: 3: search for "f" 1`] = ` +Object { + "inspectedElementID": 2, + "numElements": 3, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": 0, + "searchResults": Array [ + 2, + ], + "searchText": "f", + "selectedElementID": 2, + "selectedElementIndex": 0, +} +`; + +exports[`TreeListContext search state should find elements matching search text: 4: search for "q" 1`] = ` +Object { + "inspectedElementID": 2, + "numElements": 3, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "q", + "selectedElementID": 2, + "selectedElementIndex": 0, +} +`; + +exports[`TreeListContext search state should remove unmounted elements from the search results set: 0: mount 1`] = ` +[root] + + + +`; + +exports[`TreeListContext search state should remove unmounted elements from the search results set: 1: initial state 1`] = ` +Object { + "inspectedElementID": null, + "numElements": 3, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": null, + "selectedElementIndex": null, +} +`; + +exports[`TreeListContext search state should remove unmounted elements from the search results set: 2: search for "ba" 1`] = ` +Object { + "inspectedElementID": 3, + "numElements": 3, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": 0, + "searchResults": Array [ + 3, + 4, + ], + "searchText": "ba", + "selectedElementID": 3, + "selectedElementIndex": 1, +} +`; + +exports[`TreeListContext search state should remove unmounted elements from the search results set: 3: go to second result 1`] = ` +Object { + "inspectedElementID": 4, + "numElements": 3, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": 1, + "searchResults": Array [ + 3, + 4, + ], + "searchText": "ba", + "selectedElementID": 4, + "selectedElementIndex": 2, +} +`; + +exports[`TreeListContext search state should remove unmounted elements from the search results set: 4: unmount Baz 1`] = ` +Object { + "inspectedElementID": null, + "numElements": 2, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": 0, + "searchResults": Array [ + 3, + ], + "searchText": "ba", + "selectedElementID": null, + "selectedElementIndex": null, +} +`; + +exports[`TreeListContext search state should select the next and previous items within the search results: 0: mount 1`] = ` +[root] + + + + +`; + +exports[`TreeListContext search state should select the next and previous items within the search results: 1: initial state 1`] = ` +Object { + "inspectedElementID": null, + "numElements": 4, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": null, + "selectedElementIndex": null, +} +`; + +exports[`TreeListContext search state should select the next and previous items within the search results: 2: search for "ba" 1`] = ` +Object { + "inspectedElementID": 3, + "numElements": 4, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": 0, + "searchResults": Array [ + 3, + 4, + 5, + ], + "searchText": "ba", + "selectedElementID": 3, + "selectedElementIndex": 1, +} +`; + +exports[`TreeListContext search state should select the next and previous items within the search results: 3: go to second result 1`] = ` +Object { + "inspectedElementID": 4, + "numElements": 4, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": 1, + "searchResults": Array [ + 3, + 4, + 5, + ], + "searchText": "ba", + "selectedElementID": 4, + "selectedElementIndex": 2, +} +`; + +exports[`TreeListContext search state should select the next and previous items within the search results: 4: go to third result 1`] = ` +Object { + "inspectedElementID": 5, + "numElements": 4, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": 2, + "searchResults": Array [ + 3, + 4, + 5, + ], + "searchText": "ba", + "selectedElementID": 5, + "selectedElementIndex": 3, +} +`; + +exports[`TreeListContext search state should select the next and previous items within the search results: 5: go to second result 1`] = ` +Object { + "inspectedElementID": 4, + "numElements": 4, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": 1, + "searchResults": Array [ + 3, + 4, + 5, + ], + "searchText": "ba", + "selectedElementID": 4, + "selectedElementIndex": 2, +} +`; + +exports[`TreeListContext search state should select the next and previous items within the search results: 6: go to first result 1`] = ` +Object { + "inspectedElementID": 3, + "numElements": 4, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": 0, + "searchResults": Array [ + 3, + 4, + 5, + ], + "searchText": "ba", + "selectedElementID": 3, + "selectedElementIndex": 1, +} +`; + +exports[`TreeListContext search state should select the next and previous items within the search results: 7: wrap to last result 1`] = ` +Object { + "inspectedElementID": 5, + "numElements": 4, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": 2, + "searchResults": Array [ + 3, + 4, + 5, + ], + "searchText": "ba", + "selectedElementID": 5, + "selectedElementIndex": 3, +} +`; + +exports[`TreeListContext search state should select the next and previous items within the search results: 8: wrap to first result 1`] = ` +Object { + "inspectedElementID": 3, + "numElements": 4, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": 0, + "searchResults": Array [ + 3, + 4, + 5, + ], + "searchText": "ba", + "selectedElementID": 3, + "selectedElementIndex": 1, +} +`; + +exports[`TreeListContext tree state should select child elements: 0: mount 1`] = ` +[root] + ▾ + ▾ + + + ▾ + + +`; + +exports[`TreeListContext tree state should select child elements: 1: initial state 1`] = ` +Object { + "inspectedElementID": null, + "numElements": 7, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": null, + "selectedElementIndex": null, +} +`; + +exports[`TreeListContext tree state should select child elements: 2: select first element 1`] = ` +Object { + "inspectedElementID": 2, + "numElements": 7, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": 2, + "selectedElementIndex": 0, +} +`; + +exports[`TreeListContext tree state should select child elements: 3: select Parent 1`] = ` +Object { + "inspectedElementID": 3, + "numElements": 7, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": 3, + "selectedElementIndex": 1, +} +`; + +exports[`TreeListContext tree state should select child elements: 4: select Child 1`] = ` +Object { + "inspectedElementID": 4, + "numElements": 7, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": 4, + "selectedElementIndex": 2, +} +`; + +exports[`TreeListContext tree state should select parent elements and then collapse: 0: mount 1`] = ` +[root] + ▾ + ▾ + + + ▾ + + +`; + +exports[`TreeListContext tree state should select parent elements and then collapse: 1: initial state 1`] = ` +Object { + "inspectedElementID": null, + "numElements": 7, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": null, + "selectedElementIndex": null, +} +`; + +exports[`TreeListContext tree state should select parent elements and then collapse: 2: select last child 1`] = ` +Object { + "inspectedElementID": 8, + "numElements": 7, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": 8, + "selectedElementIndex": 6, +} +`; + +exports[`TreeListContext tree state should select parent elements and then collapse: 3: select Parent 1`] = ` +Object { + "inspectedElementID": 6, + "numElements": 7, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": 6, + "selectedElementIndex": 4, +} +`; + +exports[`TreeListContext tree state should select parent elements and then collapse: 4: select Grandparent 1`] = ` +Object { + "inspectedElementID": 2, + "numElements": 7, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": 2, + "selectedElementIndex": 0, +} +`; + +exports[`TreeListContext tree state should select the next and previous elements in the tree: 0: mount 1`] = ` +[root] + ▾ + ▾ + + +`; + +exports[`TreeListContext tree state should select the next and previous elements in the tree: 1: initial state 1`] = ` +Object { + "inspectedElementID": null, + "numElements": 4, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": null, + "selectedElementIndex": null, +} +`; + +exports[`TreeListContext tree state should select the next and previous elements in the tree: 2: select first element 1`] = ` +Object { + "inspectedElementID": 2, + "numElements": 4, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": 2, + "selectedElementIndex": 0, +} +`; + +exports[`TreeListContext tree state should select the next and previous elements in the tree: 3: select element after (0) 1`] = ` +Object { + "inspectedElementID": 3, + "numElements": 4, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": 3, + "selectedElementIndex": 1, +} +`; + +exports[`TreeListContext tree state should select the next and previous elements in the tree: 3: select element after (1) 1`] = ` +Object { + "inspectedElementID": 4, + "numElements": 4, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": 4, + "selectedElementIndex": 2, +} +`; + +exports[`TreeListContext tree state should select the next and previous elements in the tree: 3: select element after (2) 1`] = ` +Object { + "inspectedElementID": 5, + "numElements": 4, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": 5, + "selectedElementIndex": 3, +} +`; + +exports[`TreeListContext tree state should select the next and previous elements in the tree: 4: select element before (1) 1`] = ` +Object { + "inspectedElementID": 2, + "numElements": 4, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": 2, + "selectedElementIndex": 0, +} +`; + +exports[`TreeListContext tree state should select the next and previous elements in the tree: 4: select element before (2) 1`] = ` +Object { + "inspectedElementID": 3, + "numElements": 4, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": 3, + "selectedElementIndex": 1, +} +`; + +exports[`TreeListContext tree state should select the next and previous elements in the tree: 4: select element before (3) 1`] = ` +Object { + "inspectedElementID": 4, + "numElements": 4, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": 4, + "selectedElementIndex": 2, +} +`; + +exports[`TreeListContext tree state should select the next and previous elements in the tree: 5: select previous wraps around to last 1`] = ` +Object { + "inspectedElementID": 5, + "numElements": 4, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": 5, + "selectedElementIndex": 3, +} +`; + +exports[`TreeListContext tree state should select the next and previous elements in the tree: 6: select next wraps around to first 1`] = ` +Object { + "inspectedElementID": 2, + "numElements": 4, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": 2, + "selectedElementIndex": 0, +} +`; diff --git a/src/__tests__/ownersListContext-test.js b/src/__tests__/ownersListContext-test.js index 348f6932c4..06464e663a 100644 --- a/src/__tests__/ownersListContext-test.js +++ b/src/__tests__/ownersListContext-test.js @@ -53,15 +53,15 @@ describe('OwnersListContext', () => { it('should fetch the owners list for the selected element', async done => { const Grandparent = () => ; - const Parent = ({ count }) => { + const Parent = () => { return ( - - + + ); }; - const Child = ({ duration }) => null; + const Child = () => null; utils.act(() => ReactDOM.render(, document.createElement('div')) @@ -118,15 +118,15 @@ describe('OwnersListContext', () => { store.componentFilters = [utils.createDisplayNameFilter('^Parent$')]; const Grandparent = () => ; - const Parent = ({ count }) => { + const Parent = () => { return ( - - + + ); }; - const Child = ({ duration }) => null; + const Child = () => null; utils.act(() => ReactDOM.render(, document.createElement('div')) diff --git a/src/__tests__/treeContext-test.js b/src/__tests__/treeContext-test.js new file mode 100644 index 0000000000..0c8f15d4b0 --- /dev/null +++ b/src/__tests__/treeContext-test.js @@ -0,0 +1,428 @@ +// @flow + +import typeof ReactTestRenderer from 'react-test-renderer'; +import type Bridge from 'src/bridge'; +import type Store from 'src/devtools/store'; +import type { + DispatcherContext, + StateContext, +} from 'src/devtools/views/Components/TreeContext'; + +describe('TreeListContext', () => { + let React; + let ReactDOM; + let TestRenderer: ReactTestRenderer; + let bridge: Bridge; + let store: Store; + let utils; + + let BridgeContext; + let StoreContext; + let TreeContext; + + let dispatch: DispatcherContext; + let state: StateContext; + + beforeEach(() => { + utils = require('./utils'); + utils.beforeEachProfiling(); + + bridge = global.bridge; + store = global.store; + store.collapseNodesByDefault = false; + + React = require('react'); + ReactDOM = require('react-dom'); + TestRenderer = utils.requireTestRenderer(); + + BridgeContext = require('src/devtools/views/context').BridgeContext; + StoreContext = require('src/devtools/views/context').StoreContext; + TreeContext = require('src/devtools/views/Components/TreeContext'); + }); + + afterEach(() => { + // Reset between tests + dispatch = ((null: any): DispatcherContext); + state = ((null: any): StateContext); + }); + + const Capture = () => { + dispatch = React.useContext(TreeContext.TreeDispatcherContext); + state = React.useContext(TreeContext.TreeStateContext); + return null; + }; + + const Contexts = () => { + return ( + + + + + + + + ); + }; + + describe('tree state', () => { + it('should select the next and previous elements in the tree', () => { + const Grandparent = () => ; + const Parent = () => ( + + + + + ); + const Child = () => null; + + utils.act(() => + ReactDOM.render(, document.createElement('div')) + ); + + expect(store).toMatchSnapshot('0: mount'); + + let renderer; + utils.act(() => (renderer = TestRenderer.create())); + expect(state).toMatchSnapshot('1: initial state'); + + utils.act(() => dispatch({ type: 'SELECT_NEXT_ELEMENT_IN_TREE' })); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('2: select first element'); + + while ( + state.selectedElementIndex !== null && + state.selectedElementIndex < store.numElements - 1 + ) { + const index = ((state.selectedElementIndex: any): number); + utils.act(() => dispatch({ type: 'SELECT_NEXT_ELEMENT_IN_TREE' })); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot(`3: select element after (${index})`); + } + + while ( + state.selectedElementIndex !== null && + state.selectedElementIndex > 0 + ) { + const index = ((state.selectedElementIndex: any): number); + utils.act(() => dispatch({ type: 'SELECT_PREVIOUS_ELEMENT_IN_TREE' })); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot(`4: select element before (${index})`); + } + + utils.act(() => dispatch({ type: 'SELECT_PREVIOUS_ELEMENT_IN_TREE' })); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('5: select previous wraps around to last'); + + utils.act(() => dispatch({ type: 'SELECT_NEXT_ELEMENT_IN_TREE' })); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('6: select next wraps around to first'); + }); + + it('should select child elements', () => { + const Grandparent = () => ( + + + + + ); + const Parent = () => ( + + + + + ); + const Child = () => null; + + utils.act(() => + ReactDOM.render(, document.createElement('div')) + ); + + expect(store).toMatchSnapshot('0: mount'); + + let renderer; + utils.act(() => (renderer = TestRenderer.create())); + expect(state).toMatchSnapshot('1: initial state'); + + utils.act(() => + dispatch({ type: 'SELECT_ELEMENT_AT_INDEX', payload: 0 }) + ); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('2: select first element'); + + utils.act(() => dispatch({ type: 'SELECT_CHILD_ELEMENT_IN_TREE' })); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('3: select Parent'); + + utils.act(() => dispatch({ type: 'SELECT_CHILD_ELEMENT_IN_TREE' })); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('4: select Child'); + + const previousState = state; + + // There are no more children to select, so this should be a no-op + utils.act(() => dispatch({ type: 'SELECT_CHILD_ELEMENT_IN_TREE' })); + utils.act(() => renderer.update()); + expect(state).toEqual(previousState); + }); + + it('should select parent elements and then collapse', () => { + const Grandparent = () => ( + + + + + ); + const Parent = () => ( + + + + + ); + const Child = () => null; + + utils.act(() => + ReactDOM.render(, document.createElement('div')) + ); + + expect(store).toMatchSnapshot('0: mount'); + + let renderer; + utils.act(() => (renderer = TestRenderer.create())); + expect(state).toMatchSnapshot('1: initial state'); + + const lastChildID = store.getElementIDAtIndex(store.numElements - 1); + + utils.act(() => + dispatch({ type: 'SELECT_ELEMENT_BY_ID', payload: lastChildID }) + ); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('2: select last child'); + + utils.act(() => dispatch({ type: 'SELECT_PARENT_ELEMENT_IN_TREE' })); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('3: select Parent'); + + utils.act(() => dispatch({ type: 'SELECT_PARENT_ELEMENT_IN_TREE' })); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('4: select Grandparent'); + + const previousState = state; + + // There are no more ancestors to select, so this should be a no-op + utils.act(() => dispatch({ type: 'SELECT_PARENT_ELEMENT_IN_TREE' })); + utils.act(() => renderer.update()); + expect(state).toEqual(previousState); + }); + }); + + describe('search state', () => { + it('should find elements matching search text', () => { + const Foo = () => null; + const Bar = () => null; + const Baz = () => null; + + utils.act(() => + ReactDOM.render( + + + + + , + document.createElement('div') + ) + ); + + expect(store).toMatchSnapshot('0: mount'); + + let renderer; + utils.act(() => (renderer = TestRenderer.create())); + expect(state).toMatchSnapshot('1: initial state'); + + utils.act(() => dispatch({ type: 'SET_SEARCH_TEXT', payload: 'ba' })); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('2: search for "ba"'); + + utils.act(() => dispatch({ type: 'SET_SEARCH_TEXT', payload: 'f' })); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('3: search for "f"'); + + utils.act(() => dispatch({ type: 'SET_SEARCH_TEXT', payload: 'q' })); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('4: search for "q"'); + }); + + it('should select the next and previous items within the search results', () => { + const Foo = () => null; + const Bar = () => null; + const Baz = () => null; + + utils.act(() => + ReactDOM.render( + + + + + + , + document.createElement('div') + ) + ); + + expect(store).toMatchSnapshot('0: mount'); + + let renderer; + utils.act(() => (renderer = TestRenderer.create())); + expect(state).toMatchSnapshot('1: initial state'); + + utils.act(() => dispatch({ type: 'SET_SEARCH_TEXT', payload: 'ba' })); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('2: search for "ba"'); + + utils.act(() => dispatch({ type: 'GO_TO_NEXT_SEARCH_RESULT' })); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('3: go to second result'); + + utils.act(() => dispatch({ type: 'GO_TO_NEXT_SEARCH_RESULT' })); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('4: go to third result'); + + utils.act(() => dispatch({ type: 'GO_TO_PREVIOUS_SEARCH_RESULT' })); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('5: go to second result'); + + utils.act(() => dispatch({ type: 'GO_TO_PREVIOUS_SEARCH_RESULT' })); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('6: go to first result'); + + utils.act(() => dispatch({ type: 'GO_TO_PREVIOUS_SEARCH_RESULT' })); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('7: wrap to last result'); + + utils.act(() => dispatch({ type: 'GO_TO_NEXT_SEARCH_RESULT' })); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('8: wrap to first result'); + }); + + it('should add newly mounted elements to the search results set if they match the current text', async done => { + const Foo = () => null; + const Bar = () => null; + const Baz = () => null; + + const container = document.createElement('div'); + + utils.act(() => + ReactDOM.render( + + + + , + container + ) + ); + + expect(store).toMatchSnapshot('0: mount'); + + let renderer; + utils.act(() => (renderer = TestRenderer.create())); + expect(state).toMatchSnapshot('1: initial state'); + + utils.act(() => dispatch({ type: 'SET_SEARCH_TEXT', payload: 'ba' })); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('2: search for "ba"'); + + await utils.actSuspense(() => + ReactDOM.render( + + + + + , + container + ) + ); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('3: mount Baz'); + + done(); + }); + + it('should remove unmounted elements from the search results set', async done => { + const Foo = () => null; + const Bar = () => null; + const Baz = () => null; + + const container = document.createElement('div'); + + utils.act(() => + ReactDOM.render( + + + + + , + container + ) + ); + + expect(store).toMatchSnapshot('0: mount'); + + let renderer; + utils.act(() => (renderer = TestRenderer.create())); + expect(state).toMatchSnapshot('1: initial state'); + + utils.act(() => dispatch({ type: 'SET_SEARCH_TEXT', payload: 'ba' })); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('2: search for "ba"'); + + utils.act(() => dispatch({ type: 'GO_TO_NEXT_SEARCH_RESULT' })); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('3: go to second result'); + + await utils.actSuspense(() => + ReactDOM.render( + + + + , + container + ) + ); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('4: unmount Baz'); + + done(); + }); + }); + + describe('owners state', () => { + it('should support entering and existing the owners tree view', () => { + const Grandparent = () => ; + const Parent = () => ( + + + + + ); + const Child = () => null; + + utils.act(() => + ReactDOM.render(, document.createElement('div')) + ); + + expect(store).toMatchSnapshot('0: mount'); + + let renderer; + utils.act(() => (renderer = TestRenderer.create())); + expect(state).toMatchSnapshot('1: initial state'); + + let parentID = ((store.getElementIDAtIndex(1): any): number); + utils.act(() => dispatch({ type: 'SELECT_OWNER', payload: parentID })); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('2: parent owners tree'); + + utils.act(() => dispatch({ type: 'RESET_OWNER_STACK' })); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('3: final state'); + }); + }); +}); diff --git a/src/devtools/views/Components/TreeContext.js b/src/devtools/views/Components/TreeContext.js index 6f65e7920d..1952497cbd 100644 --- a/src/devtools/views/Components/TreeContext.js +++ b/src/devtools/views/Components/TreeContext.js @@ -38,7 +38,7 @@ import Store from '../../store'; import type { Element } from './types'; -type StateContext = {| +export type StateContext = {| // Tree numElements: number, selectedElementID: number | null, @@ -117,7 +117,7 @@ type Action = | ACTION_SET_SEARCH_TEXT | ACTION_UPDATE_INSPECTED_ELEMENT_ID; -type DispatcherContext = (action: Action) => void; +export type DispatcherContext = (action: Action) => void; const TreeStateContext = createContext( ((null: any): StateContext) From 7160f6f584d3eb487230fe64b4a76e5086eb7cec Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 9 May 2019 18:11:17 -0700 Subject: [PATCH 5/6] Fixed owners stack direction and added current element to stack --- .../ownersListContext-test.js.snap | 34 ++++++++++++-- src/__tests__/ownersListContext-test.js | 44 +++++++++++++++++++ src/backend/renderer.js | 27 ++++++++++-- src/devtools/views/Components/OwnersStack.js | 12 ++--- .../views/Components/SelectedElement.js | 2 +- src/devtools/views/Components/types.js | 2 +- 6 files changed, 106 insertions(+), 15 deletions(-) diff --git a/src/__tests__/__snapshots__/ownersListContext-test.js.snap b/src/__tests__/__snapshots__/ownersListContext-test.js.snap index 74c908afe8..0f1e213a05 100644 --- a/src/__tests__/__snapshots__/ownersListContext-test.js.snap +++ b/src/__tests__/__snapshots__/ownersListContext-test.js.snap @@ -9,13 +9,17 @@ exports[`OwnersListContext should fetch the owners list for the selected element exports[`OwnersListContext should fetch the owners list for the selected element that includes filtered components: owners for "Child" 1`] = ` Array [ + Object { + "displayName": "Grandparent", + "id": 7, + }, Object { "displayName": "Parent", "id": 9, }, Object { - "displayName": "Grandparent", - "id": 7, + "displayName": "Child", + "id": 8, }, ] `; @@ -30,13 +34,17 @@ exports[`OwnersListContext should fetch the owners list for the selected element exports[`OwnersListContext should fetch the owners list for the selected element: owners for "Child" 1`] = ` Array [ + Object { + "displayName": "Grandparent", + "id": 2, + }, Object { "displayName": "Parent", "id": 3, }, Object { - "displayName": "Grandparent", - "id": 2, + "displayName": "Child", + "id": 4, }, ] `; @@ -47,5 +55,23 @@ Array [ "displayName": "Grandparent", "id": 2, }, + Object { + "displayName": "Parent", + "id": 3, + }, +] +`; + +exports[`OwnersListContext should include the current element even if there are no other owners: mount 1`] = ` +[root] + +`; + +exports[`OwnersListContext should include the current element even if there are no other owners: owners for "Grandparent" 1`] = ` +Array [ + Object { + "displayName": "Grandparent", + "id": 5, + }, ] `; diff --git a/src/__tests__/ownersListContext-test.js b/src/__tests__/ownersListContext-test.js index 06464e663a..bf92977ed5 100644 --- a/src/__tests__/ownersListContext-test.js +++ b/src/__tests__/ownersListContext-test.js @@ -163,4 +163,48 @@ describe('OwnersListContext', () => { done(); }); + + it('should include the current element even if there are no other owners', async done => { + store.componentFilters = [utils.createDisplayNameFilter('^Parent$')]; + + const Grandparent = () => ; + const Parent = () => null; + + utils.act(() => + ReactDOM.render(, document.createElement('div')) + ); + + expect(store).toMatchSnapshot('mount'); + + const grandparent = ((store.getElementAtIndex(0): any): Element); + + let didFinish = false; + + function Suspender({ owner }) { + const read = React.useContext(OwnersListContext); + const owners = read(owner.id); + expect(owners).toMatchSnapshot( + `owners for "${(owner && owner.displayName) || ''}"` + ); + didFinish = true; + return null; + } + + await utils.actSuspense( + () => + TestRenderer.create( + + + + + + ), + 3 + ); + expect(didFinish).toBe(true); + + done(); + }); + + // TODO (owners) Verify that if an Element in the list is unmounted the stack is updated. }); diff --git a/src/backend/renderer.js b/src/backend/renderer.js index cc49385c8f..b76ce8a7d6 100644 --- a/src/backend/renderer.js +++ b/src/backend/renderer.js @@ -1696,12 +1696,17 @@ export function attach( const { _debugOwner } = fiber; - let owners = null; + const owners = [ + { + displayName: getDisplayNameForFiber(fiber) || 'Unknown', + id, + }, + ]; + if (_debugOwner) { - owners = []; let owner = _debugOwner; while (owner !== null) { - owners.push({ + owners.unshift({ displayName: getDisplayNameForFiber(owner) || 'Unknown', id: getFiberID(getPrimaryFiber(owner)), }); @@ -1719,6 +1724,7 @@ export function attach( } const { + _debugOwner, _debugSource, stateNode, memoizedProps, @@ -1790,6 +1796,19 @@ export function attach( context = { value: context }; } + let owners = null; + if (_debugOwner) { + owners = []; + let owner = _debugOwner; + while (owner !== null) { + owners.push({ + displayName: getDisplayNameForFiber(owner) || 'Unknown', + id: getFiberID(getPrimaryFiber(owner)), + }); + owner = owner._debugOwner || null; + } + } + const isTimedOutSuspense = tag === SuspenseComponent && memoizedState !== null; @@ -1825,7 +1844,7 @@ export function attach( state: usesHooks ? null : memoizedState, // List of owners - owners: getOwnersList(id), + owners, // Location of component in source coude. source: _debugSource, diff --git a/src/devtools/views/Components/OwnersStack.js b/src/devtools/views/Components/OwnersStack.js index 77ec8eed60..3883a14480 100644 --- a/src/devtools/views/Components/OwnersStack.js +++ b/src/devtools/views/Components/OwnersStack.js @@ -45,7 +45,7 @@ type State = {| function dialogReducer(state, action) { switch (action.type) { case 'UPDATE_OWNER_ID': - const selectedIndex = state.owners.findIndex( + const selectedIndex = action.owners.findIndex( owner => owner.id === action.ownerID ); return { @@ -71,10 +71,11 @@ export default function OwnerStack() { const [state, dispatch] = useReducer(dialogReducer, { ownerID: null, owners: [], - selectedIndex: -1, + selectedIndex: 0, }); - // TODO (owners) Explain this and use reducer with ownerID too to avoid inf. loop + // When an owner is selected, we either need to update the selected index, or we need to fetch a new list of owners. + // We use a reducer here so that we can avoid fetching a new list unless the owner ID has actually changed. if (ownerID === null) { dispatch({ type: 'UPDATE_OWNER_ID', @@ -82,11 +83,12 @@ export default function OwnerStack() { owners: [], }); } else if (ownerID !== state.ownerID) { - const isInList = state.owners.findIndex(owner => owner.id === ownerID) >= 0; + const isInStore = + state.owners.findIndex(owner => owner.id === ownerID) >= 0; dispatch({ type: 'UPDATE_OWNER_ID', ownerID, - owners: isInList ? state.owners : read(ownerID) || [], + owners: isInStore ? state.owners : read(ownerID) || [], }); } diff --git a/src/devtools/views/Components/SelectedElement.js b/src/devtools/views/Components/SelectedElement.js index 4ead00425a..b49ce87ad8 100644 --- a/src/devtools/views/Components/SelectedElement.js +++ b/src/devtools/views/Components/SelectedElement.js @@ -304,7 +304,7 @@ function InspectedElementView({ {owners.map(owner => ( diff --git a/src/devtools/views/Components/types.js b/src/devtools/views/Components/types.js index 0e33349a47..aaae8d378a 100644 --- a/src/devtools/views/Components/types.js +++ b/src/devtools/views/Components/types.js @@ -31,7 +31,7 @@ export type Element = {| |}; export type Owner = {| - displayName: string, + displayName: string | null, id: number, |}; From ac0022bf4a32f8c464e59f8b71dec12f141619b3 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Fri, 10 May 2019 08:07:27 -0700 Subject: [PATCH 6/6] Added more tree context tests --- .../__snapshots__/treeContext-test.js.snap | 310 ++++++++++++++++++ src/__tests__/ownersListContext-test.js | 2 - src/__tests__/treeContext-test.js | 122 +++++++ src/devtools/views/Components/TreeContext.js | 18 +- 4 files changed, 444 insertions(+), 8 deletions(-) diff --git a/src/__tests__/__snapshots__/treeContext-test.js.snap b/src/__tests__/__snapshots__/treeContext-test.js.snap index fd9c4dbf1f..3a9149861b 100644 --- a/src/__tests__/__snapshots__/treeContext-test.js.snap +++ b/src/__tests__/__snapshots__/treeContext-test.js.snap @@ -1,5 +1,251 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[`TreeListContext owners state should exit the owners list if the current owner is unmounted: 0: mount 1`] = ` +[root] + ▾ + +`; + +exports[`TreeListContext owners state should exit the owners list if the current owner is unmounted: 1: initial state 1`] = ` +Object { + "inspectedElementID": null, + "numElements": 2, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": null, + "selectedElementIndex": null, +} +`; + +exports[`TreeListContext owners state should exit the owners list if the current owner is unmounted: 2: child owners tree 1`] = ` +Object { + "inspectedElementID": 3, + "numElements": 1, + "ownerFlatTree": Array [ + Object { + "children": Array [], + "depth": 0, + "displayName": "Child", + "id": 3, + "isCollapsed": false, + "key": null, + "ownerID": 0, + "parentID": 2, + "type": 5, + "weight": 1, + }, + ], + "ownerID": 3, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": 3, + "selectedElementIndex": 0, +} +`; + +exports[`TreeListContext owners state should exit the owners list if the current owner is unmounted: 3: remove child 1`] = ` +Object { + "inspectedElementID": null, + "numElements": 1, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": null, + "selectedElementIndex": 0, +} +`; + +exports[`TreeListContext owners state should exit the owners list if the current owner is unmounted: 4: parent owners tree 1`] = ` +Object { + "inspectedElementID": 2, + "numElements": 1, + "ownerFlatTree": Array [ + Object { + "children": Array [], + "depth": 0, + "displayName": "Parent", + "id": 2, + "isCollapsed": false, + "key": null, + "ownerID": 0, + "parentID": 1, + "type": 5, + "weight": 1, + }, + ], + "ownerID": 2, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": 2, + "selectedElementIndex": 0, +} +`; + +exports[`TreeListContext owners state should exit the owners list if the current owner is unmounted: 5: unmount root 1`] = ` +Object { + "inspectedElementID": null, + "numElements": 0, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": null, + "selectedElementIndex": 0, +} +`; + +exports[`TreeListContext owners state should remove an element from the owners list if it is unmounted: 0: mount 1`] = ` +[root] + ▾ + ▾ + + +`; + +exports[`TreeListContext owners state should remove an element from the owners list if it is unmounted: 1: initial state 1`] = ` +Object { + "inspectedElementID": null, + "numElements": 4, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": null, + "selectedElementIndex": null, +} +`; + +exports[`TreeListContext owners state should remove an element from the owners list if it is unmounted: 2: parent owners tree 1`] = ` +Object { + "inspectedElementID": 3, + "numElements": 3, + "ownerFlatTree": Array [ + Object { + "children": Array [ + 4, + 5, + ], + "depth": 0, + "displayName": "Parent", + "id": 3, + "isCollapsed": false, + "key": null, + "ownerID": 2, + "parentID": 2, + "type": 5, + "weight": 3, + }, + Object { + "children": Array [], + "depth": 1, + "displayName": "Child", + "id": 4, + "isCollapsed": false, + "key": "0", + "ownerID": 3, + "parentID": 3, + "type": 5, + "weight": 1, + }, + Object { + "children": Array [], + "depth": 1, + "displayName": "Child", + "id": 5, + "isCollapsed": false, + "key": "1", + "ownerID": 3, + "parentID": 3, + "type": 5, + "weight": 1, + }, + ], + "ownerID": 3, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": 3, + "selectedElementIndex": 0, +} +`; + +exports[`TreeListContext owners state should remove an element from the owners list if it is unmounted: 3: remove second child 1`] = ` +Object { + "inspectedElementID": 3, + "numElements": 2, + "ownerFlatTree": Array [ + Object { + "children": Array [ + 4, + ], + "depth": 0, + "displayName": "Parent", + "id": 3, + "isCollapsed": false, + "key": null, + "ownerID": 2, + "parentID": 2, + "type": 5, + "weight": 2, + }, + Object { + "children": Array [], + "depth": 1, + "displayName": "Child", + "id": 4, + "isCollapsed": false, + "key": "0", + "ownerID": 3, + "parentID": 3, + "type": 5, + "weight": 1, + }, + ], + "ownerID": 3, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": 3, + "selectedElementIndex": 0, +} +`; + +exports[`TreeListContext owners state should remove an element from the owners list if it is unmounted: 4: remove first child 1`] = ` +Object { + "inspectedElementID": 3, + "numElements": 1, + "ownerFlatTree": Array [ + Object { + "children": Array [], + "depth": 0, + "displayName": "Parent", + "id": 3, + "isCollapsed": false, + "key": null, + "ownerID": 2, + "parentID": 2, + "type": 5, + "weight": 1, + }, + ], + "ownerID": 3, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": 3, + "selectedElementIndex": 0, +} +`; + exports[`TreeListContext owners state should support entering and existing the owners tree view: 0: mount 1`] = ` [root] ▾ @@ -430,6 +676,70 @@ Object { } `; +exports[`TreeListContext tree state should clear selection if the selected element is unmounted: 0: mount 1`] = ` +[root] + ▾ + ▾ + + +`; + +exports[`TreeListContext tree state should clear selection if the selected element is unmounted: 1: initial state 1`] = ` +Object { + "inspectedElementID": null, + "numElements": 4, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": null, + "selectedElementIndex": null, +} +`; + +exports[`TreeListContext tree state should clear selection if the selected element is unmounted: 2: select second child 1`] = ` +Object { + "inspectedElementID": 5, + "numElements": 4, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": 5, + "selectedElementIndex": 3, +} +`; + +exports[`TreeListContext tree state should clear selection if the selected element is unmounted: 3: remove children (parent should now be selected) 1`] = ` +Object { + "inspectedElementID": 3, + "numElements": 2, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": 3, + "selectedElementIndex": 1, +} +`; + +exports[`TreeListContext tree state should clear selection if the selected element is unmounted: 4: unmount root (nothing should be selected) 1`] = ` +Object { + "inspectedElementID": null, + "numElements": 0, + "ownerFlatTree": null, + "ownerID": null, + "searchIndex": null, + "searchResults": Array [], + "searchText": "", + "selectedElementID": null, + "selectedElementIndex": null, +} +`; + exports[`TreeListContext tree state should select child elements: 0: mount 1`] = ` [root] ▾ diff --git a/src/__tests__/ownersListContext-test.js b/src/__tests__/ownersListContext-test.js index bf92977ed5..253e31fa3d 100644 --- a/src/__tests__/ownersListContext-test.js +++ b/src/__tests__/ownersListContext-test.js @@ -205,6 +205,4 @@ describe('OwnersListContext', () => { done(); }); - - // TODO (owners) Verify that if an Element in the list is unmounted the stack is updated. }); diff --git a/src/__tests__/treeContext-test.js b/src/__tests__/treeContext-test.js index 0c8f15d4b0..a7d7ba5094 100644 --- a/src/__tests__/treeContext-test.js +++ b/src/__tests__/treeContext-test.js @@ -213,6 +213,56 @@ describe('TreeListContext', () => { utils.act(() => renderer.update()); expect(state).toEqual(previousState); }); + + it('should clear selection if the selected element is unmounted', async done => { + const Grandparent = props => props.children || null; + const Parent = props => props.children || null; + const Child = () => null; + + const container = document.createElement('div'); + utils.act(() => + ReactDOM.render( + + + + + + , + container + ) + ); + + expect(store).toMatchSnapshot('0: mount'); + + let renderer; + utils.act(() => (renderer = TestRenderer.create())); + expect(state).toMatchSnapshot('1: initial state'); + + utils.act(() => + dispatch({ type: 'SELECT_ELEMENT_AT_INDEX', payload: 3 }) + ); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('2: select second child'); + + await utils.actSuspense(() => + ReactDOM.render( + + + , + container + ) + ); + expect(state).toMatchSnapshot( + '3: remove children (parent should now be selected)' + ); + + await utils.actSuspense(() => ReactDOM.unmountComponentAtNode(container)); + expect(state).toMatchSnapshot( + '4: unmount root (nothing should be selected)' + ); + + done(); + }); }); describe('search state', () => { @@ -424,5 +474,77 @@ describe('TreeListContext', () => { utils.act(() => renderer.update()); expect(state).toMatchSnapshot('3: final state'); }); + + it('should remove an element from the owners list if it is unmounted', async done => { + const Grandparent = ({ count }) => ; + const Parent = ({ count }) => + new Array(count).fill(true).map((_, index) => ); + const Child = () => null; + + const container = document.createElement('div'); + utils.act(() => ReactDOM.render(, container)); + + expect(store).toMatchSnapshot('0: mount'); + + let renderer; + utils.act(() => (renderer = TestRenderer.create())); + expect(state).toMatchSnapshot('1: initial state'); + + let parentID = ((store.getElementIDAtIndex(1): any): number); + utils.act(() => dispatch({ type: 'SELECT_OWNER', payload: parentID })); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('2: parent owners tree'); + + await utils.actSuspense(() => + ReactDOM.render(, container) + ); + expect(state).toMatchSnapshot('3: remove second child'); + + await utils.actSuspense(() => + ReactDOM.render(, container) + ); + expect(state).toMatchSnapshot('4: remove first child'); + + done(); + }); + + it('should exit the owners list if the current owner is unmounted', async done => { + const Parent = props => props.children || null; + const Child = () => null; + + const container = document.createElement('div'); + utils.act(() => + ReactDOM.render( + + + , + container + ) + ); + + expect(store).toMatchSnapshot('0: mount'); + + let renderer; + utils.act(() => (renderer = TestRenderer.create())); + expect(state).toMatchSnapshot('1: initial state'); + + let childID = ((store.getElementIDAtIndex(1): any): number); + utils.act(() => dispatch({ type: 'SELECT_OWNER', payload: childID })); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('2: child owners tree'); + + await utils.actSuspense(() => ReactDOM.render(, container)); + expect(state).toMatchSnapshot('3: remove child'); + + let parentID = ((store.getElementIDAtIndex(0): any): number); + utils.act(() => dispatch({ type: 'SELECT_OWNER', payload: parentID })); + utils.act(() => renderer.update()); + expect(state).toMatchSnapshot('4: parent owners tree'); + + await utils.actSuspense(() => ReactDOM.unmountComponentAtNode(container)); + expect(state).toMatchSnapshot('5: unmount root'); + + done(); + }); }); }); diff --git a/src/devtools/views/Components/TreeContext.js b/src/devtools/views/Components/TreeContext.js index 1952497cbd..067f8fbaf4 100644 --- a/src/devtools/views/Components/TreeContext.js +++ b/src/devtools/views/Components/TreeContext.js @@ -447,12 +447,18 @@ function reduceOwnersState(store: Store, state: State, action: Action): State { switch (action.type) { case 'HANDLE_STORE_MUTATION': if (ownerID !== null) { - ownerFlatTree = store.getOwnersListForElement(ownerID); - if (selectedElementID !== null) { - // Mutation might have caused the index of this ID to shift. - selectedElementIndex = ownerFlatTree.findIndex( - element => element.id === selectedElementID - ); + if (!store.containsElement(ownerID)) { + ownerID = null; + ownerFlatTree = null; + selectedElementID = null; + } else { + ownerFlatTree = store.getOwnersListForElement(ownerID); + if (selectedElementID !== null) { + // Mutation might have caused the index of this ID to shift. + selectedElementIndex = ownerFlatTree.findIndex( + element => element.id === selectedElementID + ); + } } } else { if (selectedElementID !== null) {