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.
facebook · Sep 24, 2019 · 3fe9a62 · 3fe9a62
1 parent 911104a
commit 3fe9a62
Show file tree

Hide file tree

Showing 7 changed files with 204 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,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(<Example value="foo" />, 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(<Example value="bar" />, 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(<Example value="foo" />, 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(<Example value="bar" />, 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
@@ -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(

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