Fix: Text Input when value changes externally it becomes undeletable

[jorgefilipecosta]

The sync action called when the value changes externally to synchronize the local state was making commitValue undefined.

We have this effect to call onChange when the value changes:

    React.useEffect(() => {
        return store.subscribe(
            (value) => {
                onChange(value);
            },
            (state) => state.commitValue,
            shallowCompare,
        );
    }, [onChange, store]);

If the value updates externally, and then we delete the value, onChange is not called because in that case, commitValue does not change. This PR fixes the issue by making sure commitValue is updated during the synchronization.

As an aside, it this a good practice to don't synchronize state: https://kentcdodds.com/blog/dont-sync-state-derive-it

We rely a lot on state synchronization. I guess we should try to update our components to be less dependent on the local state and more on the props that are passed to them. Props should be the source of truth.

On the TextInput component, we have two stores and three different state vars for value, previousValue, commitValue, and value. Could we remove the number of variables we have? I guess, for example, we can remove commitValue the prop value could be the commitValue. A value is committed when the prop reflects it. And then I guess we just need the local state for a temporary invalid input, e.g: if the min number is 50 we will store in this temporary location "5" to allow the user to type "50". Once a value is valid, it is committed right away, and the source of truth becomes the prop. So, in that case, we would just have a temporary value or something similar as local state. Would this work? What makes us need to have more variables?

[vercel[bot]]

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/9zj2cfv41
✅ Preview: https://g2-git-fork-jorgefilipecosta-fix-text-input-f6080f.itsjonq.vercel.app

[ItsJonQ]

If the value updates externally, and then we delete the value, onChange is not called because in that case, commitValue does not change.

I'm not entirely sure what the issue is 😅 .

Would you be able to create a very simple mockup in CodeSandbox with that edge case? https://codesandbox.io/

---

We rely a lot on state synchronization... Props should be the source of truth.

We do, but only when we have to. Unfortunately, that is often.

The reason we do this is because that often micro adjustments need to happen in regards to the state, which often passed to inner sub-components.

This simply cannot be derived (or remapped) from props.

---

In the case of TextInput, we create "phases", which use multiple stores of value. This is to allow for content to be changed (usually typing) before commiting (via something like an enter keypress or blur event).

The flow can be imagined like this:

(excuse the terrible text-art)

change -> commit -> validate -> onChange
      revert <------------|

There are multiple stages during the "commit" phase in which the value may be reverted if validation fails.

---

Hope this context helps

[jorgefilipecosta]

I'm not entirely sure what the issue is 😅 .

Would you be able to create a very simple mockup in CodeSandbox with that edge case? codesandbox.io

Hi @ItsJonQ, I included a story as part of this PR that allows us to test the issue http://localhost:6006/?path=/story/components-textinput--number-with-external-change.

Basically on text input if one deletes the field, the component calls onChange with an empty argument to remove the value. If the value property changes because of external action that behavior does not happen anymore.

Before:

Notice that even if we delete the ten value, the field becomes empty. But externally onchange was not called and the value is still 10.

After:

Onchange is called and the value is deleted even outside the component.

This issue affects the font size picker. If we change the font size using the select and then delete the font size using the input field without this fix the text input does not removes the font size selection at all.

Before:

After: