Update useEditableValue hook to sync external value changes (#16878)

* 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. * Optimistic CHANGELOG update * Added additional useEditableValue() unit test cases
facebook · Sep 25, 2019 · fa1a326 · fa1a326
1 parent 57bf275
commit fa1a326
Show file tree

Hide file tree

Showing 8 changed files with 295 additions and 83 deletions.
diff --git a/packages/react-devtools-shared/src/__tests__/useEditableValue-test.js b/packages/react-devtools-shared/src/__tests__/useEditableValue-test.js
@@ -0,0 +1,173 @@
+/**
+ * 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(<Example value={1} />, 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(<Example value={2} />, 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', () => {
+    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(<Example value={1} />, 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);
+
+    // If there ARE pending changes,
+    // an update to the external prop value should NOT override the local/pending value.
+    ReactDOM.render(<Example value={3} />, 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(<Example value={1} />, 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(<Example value={1} />, 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);
+  });
+});
diff --git 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<string | number>, value: any) => void;
 
 type EditableValueProps = {|
   className?: string,
-  initialValue: any,
   overrideValueFn: OverrideValueFn,
   path: Array<string | number>,
+  value: any,
 |};
 
 export default function EditableValue({
   className = '',
-  initialValue,
   overrideValueFn,
   path,
+  value,
 }: EditableValueProps) {
   const inputRef = useRef<HTMLInputElement | null>(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
@@ -270,9 +270,9 @@ function HookView({canEditHooks, hook, id, inspectPath, path}: HookViewProps) {
             </span>
             {typeof overrideValueFn === 'function' ? (
               <EditableValue
-                initialValue={value}
                 overrideValueFn={overrideValueFn}
                 path={[]}
+                value={value}
               />
             ) : (
               // $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
@@ -105,9 +105,9 @@ export default function InspectedElementTree({
             :&nbsp;
             <EditableValue
               className={styles.EditableValue}
-              initialValue={''}
               overrideValueFn={handleNewEntryValue}
               path={[newPropName]}
+              value={''}
             />
           </div>
         )}

diff --git 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({
         </span>
         {isEditable ? (
           <EditableValue
-            initialValue={value}
             overrideValueFn={((overrideValueFn: any): OverrideValueFn)}
             path={path}
+            value={value}
           />
         ) : (
           <span className={styles.Value}>{displayValue}</span>

diff --git 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(