ItsJonQ / g2

✨ An experimental reimagining of WordPress components
http://g2-components.com/
MIT License
105 stars 12 forks source link

Improve value/state flow for TextInput and UnitInput #232

Closed ItsJonQ closed 3 years ago

ItsJonQ commented 3 years ago

This update improves how state (value from props) is handled, transformed, and derived for controlled components, such as TextInput, UnitInput, Select, Slider, and SearchInput.

This was originally brought to light by @jorgefilipecosta in https://github.com/ItsJonQ/g2/issues/203. After ideas were shared within the Gutenberg integration PR, @saramarcondes and I took a closer look.

We started refactoring out zustand, which revealed some sync. issues with props/state. We were successful in some refactors (such as SelectDropdown), but unsuccessful in others (such as ColorPicker).

The culmination of this journey (from zustand sync to recent refactors) has highlighted a good path forward for how we can reliably handle controlled components in a simple and stable fashion.


useControlledValue()

We needed an elegant and seamless way to handle value/state sync and defaultValue fallbacks for our controlled components. The solution I came up was this:

/**
 * Simplified and improved implementation of useControlledState.
 *
 * @param {object} props
 * @param {string|number} [props.defaultValue]
 * @param {string|number} [props.value]
 * @param {() => void} [props.onChange]
 */
export function useControlledValue({
    defaultValue,
    onChange,
    value: valueProp,
}) {
    const hasValue = !isNil(valueProp);
    const initialValue = hasValue ? valueProp : defaultValue;

    const [state, setState] = React.useState(initialValue);

    const value = hasValue ? valueProp : state;
    const setValue = hasValue && !isNil(onChange) ? onChange : setState;

    return [value, setValue];
}

It's the successor to useControlledState. It's pretty simple! If value is defined, use that. Otherwise, fallback to state. (Same for onChange).


usePropRef()

Another improvement is the addition of usePropRef. This simulates the "selector" benefits that zustand provided for callback functions. By creating a ref for a collection of props, this collection can be passed into useCallback based functions without breaking dependency equality.

For TextInput, this ensures that increment and decrement doesn't change per render cycle. This is useful as actions (and others) are used throughout the component.

/**
 * Creates a reference for a prop. This is useful for preserving dependency
 * memoization for hooks like useCallback.
 *
 * @example
 * ```js
 * // Referencing a simple prop, used in a useCallback function.
 * const valueRef = usePropRef(value)
 *
 * const increment = React.useCallback(() => {
 *   const value = valueRef.current
 *   onChange(value + 1)
 * }, [onChange, valueRef])
 * ```
 *
 * ---
 *
 * Multiple props can be passed in using in `object`.
 *
 * @example
 * ```js
 * const propRefs = usePropRef({ value, step })
 *
 * const increment = React.useCallback(() => {
 *   const { value, step } = propRefs.current
 *   onChange(value + step)
 * }, [onChange, propRefs])
 * ```
 */
export function usePropRef(prop) {
    const propRef = useRef(prop);

    useEffect(() => {
        propRef.current = prop;
    }, [prop]);

    return propRef;
}

From that, I refactored the previous zustand setup actions, selectors, etc... to callback functions and derived values (suggested by @jorgefilipecosta ).

From my tests, it looks like it worked!


Demo

The easiest place to test would be here: https://g2-git-try-refactor-text-input-prop-state-handling.itsjonq.vercel.app/iframe.html?id=components-unitinput--default&viewMode=story


Examples

I've created a couple of CodeSandbox examples for the 2 new hooks

useControlledValue https://codesandbox.io/s/use-controlled-value-kdpcg?file=/src/App.js

usePropRef https://codesandbox.io/s/use-prop-ref-examples-4eowp


The big ones, that being TextInput and UnitInput, look like they're working as expected.

I suspect with these changes, we'll be able to complete the ColorPicker refactor. From my tests, I suspect the infinite looping issue is being caused by incompatible render cycles between controlled components.

🙏

vercel[bot] commented 3 years ago

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/itsjonq/g2/8m40t6zyb
✅ Preview: https://g2-git-try-refactor-text-input-prop-state-handling.itsjonq.vercel.app

sarayourfriend commented 3 years ago

Good work sorting out a generic solution for this.