Skip to content

Commit

Permalink
Update useEditableValue hook to sync external value changes (#16878)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
Brian Vaughn authored Sep 25, 2019
1 parent 57bf275 commit fa1a326
Show file tree
Hide file tree
Showing 8 changed files with 295 additions and 83 deletions.
173 changes: 173 additions & 0 deletions packages/react-devtools-shared/src/__tests__/useEditableValue-test.js
Original file line number Diff line number Diff line change
@@ -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);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,9 @@ export default function InspectedElementTree({
:&nbsp;
<EditableValue
className={styles.EditableValue}
initialValue={''}
overrideValueFn={handleNewEntryValue}
path={[newPropName]}
value={''}
/>
</div>
)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -619,6 +619,7 @@ type Props = {|
children: React$Node,

// Used for automated testing
defaultInspectedElementID?: ?number,
defaultOwnerID?: ?number,
defaultSelectedElementID?: ?number,
defaultSelectedElementIndex?: ?number,
Expand All @@ -627,6 +628,7 @@ type Props = {|
// TODO Remove TreeContextController wrapper element once global ConsearchText.write API exists.
function TreeContextController({
children,
defaultInspectedElementID,
defaultOwnerID,
defaultSelectedElementID,
defaultSelectedElementIndex,
Expand Down Expand Up @@ -700,7 +702,8 @@ function TreeContextController({
ownerFlatTree: null,

// Inspection element panel
inspectedElementID: null,
inspectedElementID:
defaultInspectedElementID == null ? null : defaultInspectedElementID,
});

const dispatchWrapper = useCallback(
Expand Down
Loading

0 comments on commit fa1a326

Please sign in to comment.