final-form / react-final-form

🏁 High performance subscription-based form state management for React
https://final-form.org/react
MIT License
7.39k stars 481 forks source link

Controlled/Uncontrolled warning when using formatOnBlur #529

Open lydell opened 5 years ago

lydell commented 5 years ago

Are you submitting a bug report or a feature request?

Bug report. (I think!)

What is the current behavior?

I use format and formatOnBlur to trim whitespace from a field when the user leaves it. I know that I must not return undefined in format.

I get this warning when typing in the field:

Warning: A component is changing an uncontrolled input of type undefined to be controlled. Input elements should not switch from uncontrolled to controlled (or vice versa). Decide between using a controlled or uncontrolled input element for the lifetime of the component.

What is the expected behavior?

No warnings!

Note that if I remove formatOnBlur the warnings do not appear.

Sandbox Link

https://codesandbox.io/embed/nervous-williams-k1ldk

What's your environment?

react-final-form: 6.1.0 final-form: 4.14.1 browser: Tried Chrome and Firefox

Other information

The code from the sandbox for convenience:

<Form
  onSubmit={values => {
    console.log("SUBMIT", values);
  }}
>
  {({ handleSubmit }) => (
    <form onSubmit={handleSubmit}>
      <Field name="name" format={(value = "") => value.trim()} formatOnBlur>
        {({ input }) => {
          return <input {...input} />;
        }}
      </Field>
    </form>
  )}
</Form>;

Also, thank you for this fantastic form solution! :100:

teseo commented 5 years ago

+1

bfricka commented 5 years ago

Strange this hasn't received a response of any kind considering this is a breaking change according to the documentation of allowNull.

By default, if your value is null, <Field/> will convert it to '', to ensure controlled inputs.

Potentially related to the discrepancies in formatOnBlur behavior described by #560

This is definitely a regression introduced in the recent refactor, as I have another project running 4.x and this is not an issue.

bfricka commented 5 years ago

Okay, after a bit of digging, this is a bit more subtle than I originally thought. The behavior of allowNull hasn't changed. In both 4.x and newer versions, it uses strict equality. I was thinking it was a change from == to === so it would catch undefined values before but no longer does. Instead the change is even more confusing:

4.1.0

// Field.js
if (formatOnBlur) {
  value = Field.defaultProps.format(value, name)
} else if (format) {
  value = format(value, name)
}

Current

// useField.js
if (formatOnBlur) {
  if (component === 'input') {
    value = defaultFormat(value, name)
  }
} else {
    value = format(value, name)
}

That's weird. So previously, you have to opt out of format if you don't want it to run (b/c it's defined in defaultProps. And with formatOnBlur, it always uses the default format in render (but the passed in format on blur). That's a wow. And now, you can't opt out of format (probably fine, though).

Now, it only runs the default format if you pass component="input", which I verified will remove the error. However, this leads to even more potential confusion, b/c of how Field orders its render checks:

if (typeof children === 'function') {
  return (children: Function)({ ...field, ...rest })
}

if (typeof component === 'string') {
  // ignore meta, combine input with any other props
  return React.createElement(component, { ...field.input, children, ...rest })
}
return renderComponent(
  { ...field, children, component, ...rest },
  `Field(${name})`
)

So, if you pass a function as child to <Field component="input" format={format} formatOnBlur>, you get the full behavior, and you won't get an uncontrolled error. But if you pass a render prop, you'll get errors b/c as you can see it just does createElement and spreads the props down (which will get spread onto the input, resulting in errors).

All of this is a bit confusing. Best case for me is allowNull uses == to catch undefined as well. IMO, there's usually not a useful distinction between undefined and null anyways, and I take a cue from Elm and use a Maybe type for anything where these distinctions aren't meaningful (e.g. type Maybe<T> = T | null | undefined;).

In the meantime, there's a few things we can do. Pass initialValue to Field, pass initialValues to Form, create a helper that normalizes input.value, etc.

Hopefully this can be meaningfully addressed, b/c it leaves the API in a strange place as is.

Update edit: This is even worse than I described. While initialValue will fix focus and blur errors, there are still errors thrown if you type something then delete the character, which makes the input value go from "" to undefined. The only way to fix this is to catch it in the input itself: <input {...input} value={input.value || ''}/>. Not lovely.

csantos1113 commented 5 years ago

Same issue Controlled/Uncontrolled warning here, the whole code in useField is super confuse

// useField.js
if (formatOnBlur) {
  if (component === 'input') {
    value = defaultFormat(value, name)
  }
} else {
    value = format(value, name)
}
  1. Why only for component === input only?
  2. Why defaultFormat and not the format from the param?
bfricka commented 5 years ago

@csantos1113 Regarding 1., my assumption is just that using component="[TagName]" was not really intended to be overloaded by passing children or a render function. It's basically for the most simple case where you don't need access to the {input, meta} or your elements at all. The fact that it "fixes" the error is likely just an unintended consequence of the rendering order (as I described) and the fact that "input" type is normalized differently than other types.

There is branching logic for normalizing other component types starting at line 212 of the latest tag (v6.3.0). It's a bit convoluted, but normalization is done by some combination of input.type and component type.

I do think this logic could be simplified greatly, however, but it would require a deeper abstraction. For now, the issue w/ formatOnBlur should be addressed at least.

justingrant commented 5 years ago

This behavior seems broken. It makes it impossible (unless you're willing to tolerate the controlled/uncontrolled warning from React) to use {...input} to spread field props into an underlying <input> element. Instead, you need to do this:

<input type="text" {...input} value={input.value || ''} />

I'm also confused why the code that @csantos1113 excerpted above. Why use defaultFormat instead of format ?

frtelg commented 2 years ago

This issue is causing problems for me when resetting a form containing an input element with formatOnBlur enabled:

        <Field format={formatPhoneNumber} formatOnBlur {...props}>
            {(fieldProps: FieldRenderProps<string>) => 
                (
                    <React.Fragment>
                        <label>{label}</label>
                        <input
                            type="text"
                            {...input}
                            value={input.value || ''}
                        />
                   </React.Fragment>
                );
            }
        </Field>

When I don't add {input.value || ''}, the input value will not be emptied.