Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass onChange/onBlur/value/name props directly to Field component instead of passing a field props #1340

Closed
VincentLanglet opened this issue Feb 18, 2019 · 10 comments
Labels

Comments

@VincentLanglet
Copy link

VincentLanglet commented Feb 18, 2019

🚀 Feature request

Current Behavior

import TextField from '@material-ui/core/TextField';
import { Field } from 'formik';

<Field name="email" type="text" label="Email" component={TextField} />

Is not working, instead I have to do

<Field name="email" type="text" label="Email" component={(props) => <TextField {...props.field} {...props} />} />

Desired Behavior

<Field name="email" type="text" label="Email" component={TextField} />

Suggested Solution

Actually, Field, FastField, etc are build this way.

    const bag = { field, form: restOfFormik };
    ...

    if (typeof component === 'string') {
      const { innerRef, ...rest } = props;
      return React.createElement(component as any, {
        ref: innerRef,
        ...field,
        ...rest,
        children,
      });
    }

    return React.createElement(component as any, {
      ...bag,
      ...props,
      children,
    });

This means CustomComponent have to use a props field and a props form to work with Field.
But some of them, like Material-UI component have well-named props (onChange, value and name) and could work directly if we passed ...field instead of field as props.

Plus, I don't think coherent the fact we pass ...field in case of component === 'string' and we pass ...bag in case of CustomComponent.

So, what about something like this (less breaking change)

    return React.createElement(component as any, {
      ...field,
      ...bag
      ...props,
      children,
    });

Or this (no duplicate props but breaking changes)

    return React.createElement(component as any, {
      ...field,
      form: restOfFormik,
      ...props,
      children,
    });

Describe alternatives you've considered

I know [formik-material-ui] exist https://github.com/stackworx/formik-material-ui
But I think it's sad to write adaptator for every material-ui component.
And this little change could avoid the use of a new library. (Less is better)

@Andreyco
Copy link
Collaborator

This change would make Formik incompatible with React Native. React Native's TextInput component signature for onChange is onChange(value: string): void.

@VincentLanglet
Copy link
Author

@Andreyco Why ? Can you give an example ?

First of all, that's not true : it's onChangeText(value: string): void and onChange(e: NativeSyntheticEvent<TextInputChangeEventData>): void;

Then, even if it was onChange(value: string): void I see no problem.
Without my suggestion, because no onChange props is filled on TextInput, you can't use

<Field name="email" type="text" label="Email" component={TextInput} />

And have to use

<Field name="email" type="text" label="Email" component={(props) => <TextInput {...props} onChange={...} />} />

This code will still work if we merge my suggestion.

@johnrom
Copy link
Collaborator

johnrom commented Feb 20, 2019

@VincentLanglet You should have to use the render function the way you described your "fix" the first time.

component={} is for components you are creating yourself according to the API described by Formik, it is not meant to accept any component. You can create a component wrapper which maps the fields from Formik.FieldProps to your component, then pass that to component={}, which is exactly how you solved the problem the first time. However, you can create this component outside of the render function, which will (probably) be better for performance.

@VincentLanglet
Copy link
Author

@johnrom I agree I can use the render function and create a custom Component wrapper ; and I will do it if the suggestion is not accepted.

component={} is for components you are creating yourself according to the API described by Formik

That's what I'm asking for. An evolution of the API described by Formik.

Cause, since the solution

return React.createElement(component as any, {
      ...field,
      ...bag
      ...props,
      children,
    });

could avoid to create wrapper, I was thinking it could be a good option to implement in Formik, without negative effect.

You could still use field/form props but you could use directly all ...field props. It could avoid to create custom component for all component using value, name, onChange, onBlur as API (like Material-UI) ; which is a pretty classic API (instead of field props)

@Andreyco
Copy link
Collaborator

Andreyco commented Feb 20, 2019

Field's component prop will pass down "correct" (I mean HTML compatible) props only for string (React own's) components, like input, select, ...

All other components are custom implementations (own code, 3rd party packaged) and it is impossible to make any library compatible with all other code in the wild. It is developers responsibility to write glue layer between Field and components of choice.

I believe Field API was/is right choice and allows it to create compatibility layers for any components. Even React Native, which is in no way DOM based. https://codesandbox.io/s/zrl24rvjw3

Proposed change is definitely breaking change. Not sure if creating, eg. DOMField and broadening API would be good decision in the long term. What do you think?

@VincentLanglet
Copy link
Author

VincentLanglet commented Feb 20, 2019

Field's component prop will pass down "correct" (I mean HTML compatible) props only for string (React own's) components, like input, select, ...

All other components are custom implementations (own code, 3rd party packaged) and it is impossible to make any library compatible with all other code in the wild. It is developers responsibility to write glue layer between Field and components of choice.

@Andreyco I'm not asking to be compatible with all other code in the wild. Just library which also use the "correct" (HTML compatible) props.

I think this two implementations should be available.

<Field name="email" type="text" label="Email" component={'input'} />
const Input = (props) => <input {...props} />

<Field name="email" type="text" label="Email" component={Input} />

Another option can be something like this,

const Input = (props) => <input {...props} />

<Field name="email" type="text" label="Email" htmlCompatibleComponent={Input} />

@Andreyco
Copy link
Collaborator

It already is compatible with intrinsic elements/components such as input or select. As soon as you introduce custom component (e.g. Input in your example), there is no way to guess expected props (it might be possible at run time, but it would cause performance penalty)

@VincentLanglet
Copy link
Author

@Andreyco I think there is a misunderstanding between us. I never talk about guessing expected props.

const Input = (props) => <input {...props} />

The specificity of my Input is that it respects the same API than input. There is nothing to guess.
Since we'll pass ...field, field and form props, it'll work without wrapper.

With the change I propose :

  • intrinsic elements/components such as input or select still works without wrapper
  • Custom component with the same API than input will work without wrapper
  • Custom component with another API will need the same wrapper than before

@jaredpalmer
Copy link
Owner

See #1371

@stale stale bot added the stale label May 7, 2019
@rushi812
Copy link

rushi812 commented Jan 8, 2020

@VincentLanglet <Field name="email" type="text" label="Email" as={TextField} /> this will work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants