From a580a8b0260bce54c3fcd1e81e470a8bce1220eb Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 24 Sep 2019 13:19:25 -0700 Subject: [PATCH 1/3] Update useEditableValue to mirror value cahnges Previously, the hook initialized local state (in useState) to mirror the prop/state value. Updates to the value were ignored though. (Once the state was initialized, it was never updated.) The new hook updates the local/editable state to mirror the external value unless there are already pending, local edits being made. --- .../src/__tests__/useEditableValue-test.js | 84 ++++++++++++ .../views/Components/EditableValue.js | 66 ++++----- .../devtools/views/Components/HooksTree.js | 2 +- .../views/Components/InspectedElementTree.js | 2 +- .../src/devtools/views/Components/KeyValue.js | 2 +- .../devtools/views/Components/TreeContext.js | 5 +- .../src/devtools/views/hooks.js | 126 +++++++++++------- 7 files changed, 204 insertions(+), 83 deletions(-) create mode 100644 packages/react-devtools-shared/src/__tests__/useEditableValue-test.js diff --git a/packages/react-devtools-shared/src/__tests__/useEditableValue-test.js b/packages/react-devtools-shared/src/__tests__/useEditableValue-test.js new file mode 100644 index 0000000000000..84f429f281a10 --- /dev/null +++ b/packages/react-devtools-shared/src/__tests__/useEditableValue-test.js @@ -0,0 +1,84 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +describe('useEditableValue', () => { + let act; + let React; + let ReactDOM; + let useEditableValue; + + beforeEach(() => { + const utils = require('./utils'); + act = utils.act; + + React = require('react'); + ReactDOM = require('react-dom'); + + useEditableValue = require('../devtools/views/hooks').useEditableValue; + }); + + it('should override editable state when external props are updated', () => { + let state; + + function Example({value}) { + const tuple = useEditableValue(value); + state = tuple[0]; + return null; + } + + const container = document.createElement('div'); + ReactDOM.render(, container); + expect(state.editableValue).toEqual('"foo"'); + expect(state.externalValue).toEqual('foo'); + expect(state.hasPendingChanges).toBe(false); + + // If there are NO pending changes, + // an update to the external prop value should override the local/pending value. + ReactDOM.render(, container); + expect(state.editableValue).toEqual('"bar"'); + expect(state.externalValue).toEqual('bar'); + expect(state.hasPendingChanges).toBe(false); + }); + + it('should not override editable state when external props are updated if there are pending changes', () => { + let dispatch, state; + + function Example({value}) { + const tuple = useEditableValue(value); + state = tuple[0]; + dispatch = tuple[1]; + return null; + } + + const container = document.createElement('div'); + ReactDOM.render(, container); + expect(state.editableValue).toEqual('"foo"'); + expect(state.externalValue).toEqual('foo'); + expect(state.hasPendingChanges).toBe(false); + + // Update (local) editable state. + act(() => + dispatch({ + type: 'UPDATE', + editableValue: 'not-foo', + externalValue: 'foo', + }), + ); + expect(state.editableValue).toEqual('not-foo'); + expect(state.externalValue).toEqual('foo'); + expect(state.hasPendingChanges).toBe(true); + + // If there ARE pending changes, + // an update to the external prop value should NOT override the local/pending value. + ReactDOM.render(, container); + expect(state.editableValue).toEqual('not-foo'); + expect(state.externalValue).toEqual('bar'); + expect(state.hasPendingChanges).toBe(true); + }); +}); diff --git a/packages/react-devtools-shared/src/devtools/views/Components/EditableValue.js b/packages/react-devtools-shared/src/devtools/views/Components/EditableValue.js index a0429bf8d37de..992f2b2dff658 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/EditableValue.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/EditableValue.js @@ -7,7 +7,7 @@ * @flow */ -import React, {Fragment, useCallback, useRef} from 'react'; +import React, {Fragment, useRef} from 'react'; import Button from '../Button'; import ButtonIcon from '../ButtonIcon'; import styles from './EditableValue.css'; @@ -17,51 +17,51 @@ type OverrideValueFn = (path: Array, value: any) => void; type EditableValueProps = {| className?: string, - initialValue: any, overrideValueFn: OverrideValueFn, path: Array, + value: any, |}; export default function EditableValue({ className = '', - initialValue, overrideValueFn, path, + value, }: EditableValueProps) { const inputRef = useRef(null); - const { - editableValue, - hasPendingChanges, - isValid, - parsedValue, - reset, - update, - } = useEditableValue(initialValue); + const [state, dispatch] = useEditableValue(value); + const {editableValue, hasPendingChanges, isValid, parsedValue} = state; - const handleChange = useCallback(({target}) => update(target.value), [ - update, - ]); + const reset = () => + dispatch({ + type: 'RESET', + externalValue: value, + }); - const handleKeyDown = useCallback( - event => { - // Prevent keydown events from e.g. change selected element in the tree - event.stopPropagation(); + const handleChange = ({target}) => + dispatch({ + type: 'UPDATE', + editableValue: target.value, + externalValue: value, + }); - switch (event.key) { - case 'Enter': - if (isValid && hasPendingChanges) { - overrideValueFn(path, parsedValue); - } - break; - case 'Escape': - reset(); - break; - default: - break; - } - }, - [hasPendingChanges, isValid, overrideValueFn, parsedValue, reset], - ); + const handleKeyDown = event => { + // Prevent keydown events from e.g. change selected element in the tree + event.stopPropagation(); + + switch (event.key) { + case 'Enter': + if (isValid && hasPendingChanges) { + overrideValueFn(path, parsedValue); + } + break; + case 'Escape': + reset(); + break; + default: + break; + } + }; let placeholder = ''; if (editableValue === undefined) { diff --git a/packages/react-devtools-shared/src/devtools/views/Components/HooksTree.js b/packages/react-devtools-shared/src/devtools/views/Components/HooksTree.js index ce0be7c47a1ef..b02e8faa95f39 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/HooksTree.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/HooksTree.js @@ -270,9 +270,9 @@ function HookView({canEditHooks, hook, id, inspectPath, path}: HookViewProps) { {typeof overrideValueFn === 'function' ? ( ) : ( // $FlowFixMe Cannot create span element because in property children diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementTree.js b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementTree.js index 9c81c3f952fdb..7a5dbae571557 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementTree.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementTree.js @@ -105,9 +105,9 @@ export default function InspectedElementTree({ :  )} diff --git a/packages/react-devtools-shared/src/devtools/views/Components/KeyValue.js b/packages/react-devtools-shared/src/devtools/views/Components/KeyValue.js index 9e82eefa426fb..76b1a2e57b897 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/KeyValue.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/KeyValue.js @@ -102,9 +102,9 @@ export default function KeyValue({ {isEditable ? ( ) : ( {displayValue} diff --git a/packages/react-devtools-shared/src/devtools/views/Components/TreeContext.js b/packages/react-devtools-shared/src/devtools/views/Components/TreeContext.js index f3f3c19948070..54738bf3d4b71 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/TreeContext.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/TreeContext.js @@ -619,6 +619,7 @@ type Props = {| children: React$Node, // Used for automated testing + defaultInspectedElementID?: ?number, defaultOwnerID?: ?number, defaultSelectedElementID?: ?number, defaultSelectedElementIndex?: ?number, @@ -627,6 +628,7 @@ type Props = {| // TODO Remove TreeContextController wrapper element once global ConsearchText.write API exists. function TreeContextController({ children, + defaultInspectedElementID, defaultOwnerID, defaultSelectedElementID, defaultSelectedElementIndex, @@ -700,7 +702,8 @@ function TreeContextController({ ownerFlatTree: null, // Inspection element panel - inspectedElementID: null, + inspectedElementID: + defaultInspectedElementID == null ? null : defaultInspectedElementID, }); const dispatchWrapper = useCallback( diff --git a/packages/react-devtools-shared/src/devtools/views/hooks.js b/packages/react-devtools-shared/src/devtools/views/hooks.js index 57ebb69f6e01c..430d2472eeb4a 100644 --- a/packages/react-devtools-shared/src/devtools/views/hooks.js +++ b/packages/react-devtools-shared/src/devtools/views/hooks.js @@ -8,68 +8,102 @@ */ import throttle from 'lodash.throttle'; -import {useCallback, useEffect, useLayoutEffect, useState} from 'react'; -import {unstable_batchedUpdates as batchedUpdates} from 'react-dom'; +import { + useCallback, + useEffect, + useLayoutEffect, + useReducer, + useState, +} from 'react'; import { localStorageGetItem, localStorageSetItem, } from 'react-devtools-shared/src/storage'; import {sanitizeForParse, smartParse, smartStringify} from '../utils'; -type EditableValue = {| +type ACTION_RESET = {| + type: 'RESET', + externalValue: any, +|}; +type ACTION_UPDATE = {| + type: 'UPDATE', + editableValue: any, + externalValue: any, +|}; + +type UseEditableValueAction = ACTION_RESET | ACTION_UPDATE; +type UseEditableValueDispatch = (action: UseEditableValueAction) => void; +type UseEditableValueState = {| editableValue: any, + externalValue: any, hasPendingChanges: boolean, isValid: boolean, parsedValue: any, - reset: () => void, - update: (newValue: any) => void, |}; +function useEditableValueReducer(state, action) { + switch (action.type) { + case 'RESET': + return { + ...state, + editableValue: smartStringify(action.externalValue), + externalValue: action.externalValue, + hasPendingChanges: false, + isValid: true, + parsedValue: action.externalValue, + }; + case 'UPDATE': + let isNewValueValid = false; + let newParsedValue; + try { + newParsedValue = smartParse(action.editableValue); + isNewValueValid = true; + } catch (error) {} + return { + ...state, + editableValue: sanitizeForParse(action.editableValue), + externalValue: action.externalValue, + hasPendingChanges: + smartStringify(action.externalValue) !== action.editableValue, + isValid: isNewValueValid, + parsedValue: isNewValueValid ? newParsedValue : state.parsedValue, + }; + default: + throw new Error(`Invalid action "${action.type}"`); + } +} + // Convenience hook for working with an editable value that is validated via JSON.parse. export function useEditableValue( - initialValue: any, - initialIsValid?: boolean = true, -): EditableValue { - const [editableValue, setEditableValue] = useState(() => - smartStringify(initialValue), - ); - const [parsedValue, setParsedValue] = useState(initialValue); - const [isValid, setIsValid] = useState(initialIsValid); + externalValue: any, +): [UseEditableValueState, UseEditableValueDispatch] { + const [state, dispatch] = useReducer< + UseEditableValueState, + UseEditableValueAction, + >(useEditableValueReducer, { + editableValue: smartStringify(externalValue), + externalValue, + hasPendingChanges: false, + isValid: true, + parsedValue: externalValue, + }); - const reset = useCallback( - () => { - setEditableValue(smartStringify(initialValue)); - setParsedValue(initialValue); - setIsValid(initialIsValid); - }, - [initialValue, initialIsValid], - ); + if (state.externalValue !== externalValue) { + if (!state.hasPendingChanges) { + dispatch({ + type: 'RESET', + externalValue, + }); + } else { + dispatch({ + type: 'UPDATE', + editableValue: state.editableValue, + externalValue, + }); + } + } - const update = useCallback(newValue => { - let isNewValueValid = false; - let newParsedValue; - try { - newParsedValue = smartParse(newValue); - isNewValueValid = true; - } catch (error) {} - - batchedUpdates(() => { - setEditableValue(sanitizeForParse(newValue)); - if (isNewValueValid) { - setParsedValue(newParsedValue); - } - setIsValid(isNewValueValid); - }); - }, []); - - return { - editableValue, - hasPendingChanges: smartStringify(initialValue) !== editableValue, - isValid, - parsedValue, - reset, - update, - }; + return [state, dispatch]; } export function useIsOverflowing( From fef22e7997c9f75681c0ccd31b884a95c4326132 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 24 Sep 2019 13:36:03 -0700 Subject: [PATCH 2/3] Optimistic CHANGELOG update --- packages/react-devtools/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/react-devtools/CHANGELOG.md b/packages/react-devtools/CHANGELOG.md index a69c50d1c4fda..efe517b2ceacf 100644 --- a/packages/react-devtools/CHANGELOG.md +++ b/packages/react-devtools/CHANGELOG.md @@ -7,9 +7,11 @@ + #### Bug fixes * Fixed bug where Components panel was always empty for certain users. ([bvaughn](https://github.com/bvaughn) in [#16864](https://github.com/facebook/react/pull/16864)) * Fixed regression in DevTools editable hooks interface that caused primitive values to be shown as `undefined`. ([bvaughn](https://github.com/bvaughn) in [#16867](https://github.com/facebook/react/pull/16867)) +* Fixed bug where DevTools showed stale values in props/state/hooks editing interface. ([bvaughn](https://github.com/bvaughn) in [#16878](https://github.com/facebook/react/pull/16878)) ## 4.1.0 (September 19, 2019) From 5ff6c1cfe611fdf70b6b5f6881400a041692f641 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 25 Sep 2019 09:58:28 -0700 Subject: [PATCH 3/3] Added additional useEditableValue() unit test cases --- .../src/__tests__/useEditableValue-test.js | 121 +++++++++++++++--- 1 file changed, 105 insertions(+), 16 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/useEditableValue-test.js b/packages/react-devtools-shared/src/__tests__/useEditableValue-test.js index 84f429f281a10..cdca311cb84fe 100644 --- a/packages/react-devtools-shared/src/__tests__/useEditableValue-test.js +++ b/packages/react-devtools-shared/src/__tests__/useEditableValue-test.js @@ -33,17 +33,21 @@ describe('useEditableValue', () => { } const container = document.createElement('div'); - ReactDOM.render(, container); - expect(state.editableValue).toEqual('"foo"'); - expect(state.externalValue).toEqual('foo'); + ReactDOM.render(, container); + expect(state.editableValue).toEqual('1'); + expect(state.externalValue).toEqual(1); + expect(state.parsedValue).toEqual(1); expect(state.hasPendingChanges).toBe(false); + expect(state.isValid).toBe(true); // If there are NO pending changes, // an update to the external prop value should override the local/pending value. - ReactDOM.render(, container); - expect(state.editableValue).toEqual('"bar"'); - expect(state.externalValue).toEqual('bar'); + ReactDOM.render(, container); + expect(state.editableValue).toEqual('2'); + expect(state.externalValue).toEqual(2); + expect(state.parsedValue).toEqual(2); expect(state.hasPendingChanges).toBe(false); + expect(state.isValid).toBe(true); }); it('should not override editable state when external props are updated if there are pending changes', () => { @@ -57,28 +61,113 @@ describe('useEditableValue', () => { } const container = document.createElement('div'); - ReactDOM.render(, container); - expect(state.editableValue).toEqual('"foo"'); - expect(state.externalValue).toEqual('foo'); + ReactDOM.render(, container); + expect(state.editableValue).toEqual('1'); + expect(state.externalValue).toEqual(1); + expect(state.parsedValue).toEqual(1); expect(state.hasPendingChanges).toBe(false); + expect(state.isValid).toBe(true); // Update (local) editable state. act(() => dispatch({ type: 'UPDATE', - editableValue: 'not-foo', - externalValue: 'foo', + editableValue: '2', + externalValue: 1, }), ); - expect(state.editableValue).toEqual('not-foo'); - expect(state.externalValue).toEqual('foo'); + expect(state.editableValue).toEqual('2'); + expect(state.externalValue).toEqual(1); + expect(state.parsedValue).toEqual(2); expect(state.hasPendingChanges).toBe(true); + expect(state.isValid).toBe(true); // If there ARE pending changes, // an update to the external prop value should NOT override the local/pending value. - ReactDOM.render(, container); - expect(state.editableValue).toEqual('not-foo'); - expect(state.externalValue).toEqual('bar'); + ReactDOM.render(, container); + expect(state.editableValue).toEqual('2'); + expect(state.externalValue).toEqual(3); + expect(state.parsedValue).toEqual(2); expect(state.hasPendingChanges).toBe(true); + expect(state.isValid).toBe(true); + }); + + it('should parse edits to ensure valid JSON', () => { + let dispatch, state; + + function Example({value}) { + const tuple = useEditableValue(value); + state = tuple[0]; + dispatch = tuple[1]; + return null; + } + + const container = document.createElement('div'); + ReactDOM.render(, container); + expect(state.editableValue).toEqual('1'); + expect(state.externalValue).toEqual(1); + expect(state.parsedValue).toEqual(1); + expect(state.hasPendingChanges).toBe(false); + expect(state.isValid).toBe(true); + + // Update (local) editable state. + act(() => + dispatch({ + type: 'UPDATE', + editableValue: '"a', + externalValue: 1, + }), + ); + expect(state.editableValue).toEqual('"a'); + expect(state.externalValue).toEqual(1); + expect(state.parsedValue).toEqual(1); + expect(state.hasPendingChanges).toBe(true); + expect(state.isValid).toBe(false); + }); + + it('should reset to external value upon request', () => { + let dispatch, state; + + function Example({value}) { + const tuple = useEditableValue(value); + state = tuple[0]; + dispatch = tuple[1]; + return null; + } + + const container = document.createElement('div'); + ReactDOM.render(, container); + expect(state.editableValue).toEqual('1'); + expect(state.externalValue).toEqual(1); + expect(state.parsedValue).toEqual(1); + expect(state.hasPendingChanges).toBe(false); + expect(state.isValid).toBe(true); + + // Update (local) editable state. + act(() => + dispatch({ + type: 'UPDATE', + editableValue: '2', + externalValue: 1, + }), + ); + expect(state.editableValue).toEqual('2'); + expect(state.externalValue).toEqual(1); + expect(state.parsedValue).toEqual(2); + expect(state.hasPendingChanges).toBe(true); + expect(state.isValid).toBe(true); + + // Reset editable state + act(() => + dispatch({ + type: 'RESET', + externalValue: 1, + }), + ); + expect(state.editableValue).toEqual('1'); + expect(state.externalValue).toEqual(1); + expect(state.parsedValue).toEqual(1); + expect(state.hasPendingChanges).toBe(false); + expect(state.isValid).toBe(true); }); });