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

RFC: Improving Field #1371

Closed
jaredpalmer opened this issue Mar 8, 2019 · 32 comments
Closed

RFC: Improving Field #1371

jaredpalmer opened this issue Mar 8, 2019 · 32 comments

Comments

@jaredpalmer
Copy link
Owner

jaredpalmer commented Mar 8, 2019

I want to take folk's temperature on some warts that I want to change related to <Field> as well as discuss how best to move forward in a way that doesn't force people to rewrite stuff immediately.

<Field component>

This prop is the source of a ton of confusion because it does not simply pass through onChange, onBlur, and value but rather field and form. This means that there is some inconsistent behavior with respect to <Field component="select"> vs. <Field component={MyInput}>. I think a decent solution is to create a new prop called is or as and make that prop just pass through onBlur, onChange, value/checked as expected. We then would put a deprecation warning on component instructing people to use as/is or children and eventually remove component entirely in future versions. Unfortunately, this will force folks to change every single "select" and "textarea" over time, but this at least avoids breaking changes

<Field render>

This is just redundant at this point. We should put up a deprecation warning suggesting to use children as a function.

<Field children>

We should merge #343 and add a meta to <Field children>.

<Field>
-  {({field, form }) => ... }
+  {({field, form, meta }) => ... }
</Field>

Next Steps

In a perfect world, we make these changes gradually in a way that prevents people from having to rewrite stuff. I definitely don't want to have to rewrite all my inputs, but we need to make these changes at some point. We also need to consider that <Field> will have a dramatically reduced usage in the future once the useField hook is released.

Umbrella Issues

@VincentLanglet
Copy link

I agree passing onChange, onBlur, value and name directly instead of inside a field props is less confusing. It will be similar to component given as string and add more compatibility with other component, like Material-ui. Actually we have to do:

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

But, it's not clear for me.
Do you still consider passing the form props or not ? I think this one can still be useful.

@WickyNilliams
Copy link

Sounds good to me. Would be simpler (or unnecessary) to adapt existing components to Formik

@Andreyco
Copy link
Collaborator

Andreyco commented Mar 9, 2019

@VincentLanglet

But, it's not clear for me.
Do you still consider passing the form props or not ? I think this one can still be useful.

Based on original post it is most likely a NO. onChange, onBlur and value props only. Passing form is redundant.

@jaredpalmer
Copy link
Owner Author

Correct

@VincentLanglet
Copy link

I found useful to have access to form.touched[field.name] && form.errors[field.name]

@Andreyco
Copy link
Collaborator

If you find useful to access form.{whatever} you are creating intermediate components that knows too much. Such components are not compatible with intrinsic components which would pass all props down (React DOM warns about this) and those would not make use of form prop at all, unless they know about form prop.

This is actually powerful pattern which allows us to use Formik with UI libraries of out choice with small amount of work.

@VincentLanglet
Copy link

Sorry, I'm not sure to understand.

What is/will be the good pattern for this kind of component ?

import TextField from '@material-ui/core/TextField';
import React from 'react';

const FormField = ({ field, form, ...otherProps }) => {
  const error = form.touched[field.name] && form.errors[field.name];

  return <TextField {...field} {...otherProps} error={!!error} helperText={error} />;
};

const LoginForm = () => (
  <Form>
    <Field name="email" type="text" label="Email" variant="outlined" component={FormField} />
  </Form>
);

Not using Field ?

@Andreyco
Copy link
Collaborator

Yes. And you use Field in LoginForm scope ;)

Also, you can compose your FormField component this way

import TextField from '@material-ui/core/TextField';
import React from 'react';

const FormField = props => {
  return (
    <Field {...props}>
      {({ field, form, meta, ...otherProps }) => {
        const error = form.touched[field.name] && form.errors[field.name];
        return (
          <TextField
            {...field}
            {...otherProps}
            error={!!error}
            helperText={error}
          />
        );
      }}
    </Field>
  );
};

const LoginForm = () => (
  <Form>
    <FormField name="email" type="text" label="Email" variant="outlined" />
  </Form>
);

@johnrom
Copy link
Collaborator

johnrom commented Mar 11, 2019

This will have an added bonus of resolving types on the field, as form is actually a valid input attribute that specifies to which form an input belongs.

@jackjocross
Copy link
Contributor

This seems like the right API to me. I also think that with a useField, the main use case for <Field /> is to pass third party components in directly which this API makes much easier.

@jaredpalmer
Copy link
Owner Author

Do we prefer is or as?

@WickyNilliams
Copy link

Personally I'd say as. It aligns with styled-components. And is has some OOP inheritance connotations in my mind

@Andreyco
Copy link
Collaborator

as pls

@jaredpalmer
Copy link
Owner Author

Implemented in v2

@jaredpalmer jaredpalmer changed the title RFC: Improving Field (breaking changes) RFC: Improving Field Apr 2, 2019
jaredpalmer added a commit that referenced this issue Apr 2, 2019
…1406)

- Deprecate `<Field component>` and `<Field render>`. These haven't been removed. However, they will yield a warning in the console. 
- Add `<Field as>` prop which just passes related handlers and props straight through. This should make Styled Components folks happy.

Related to #1371
Closes #1407 
Closes #1408
@klis87
Copy link
Contributor

klis87 commented Jun 1, 2019

I have a question regarding deprecation of component and render props. I totally agree with render deprecation. Native children does exactly the same so render can go away. But component? This is very much different and I don't understand why it is going to be removed? Imagine React Router without component prop for Route. Render prop is not always the most convenient.

@jaredpalmer I have so many custom components in my apps ready to be passed as component to Field. Things like VincentLanglet mentioned. And yes, we could solve it as @Andreyco recommended to have Field inside custom components scope, but this has following issues:

  • it introduces another component layer, for forms with 100+ fields this can really make a difference
  • having field inside custom component like TextField won't allow us to decide between using Field and FastField with it. Yeah, we could make things like FieldComponent = props.useFast ? FastField : Field, but it is not the most convenient to write inside each reusable field
  • we could make another type of abstraction without component prop, instead of writing TextField component, we could just create renderTextField function, but this is bad too, for instance you can apply HOC to TextField, but u cannot do it for renderTextField, function is NOT the functional component. To use HOC in renderTextField, we would need to create another component with HOC outside of renderTextField, which again would cause introducing another layer in React component tree and inconvenience.

So to sum up, please, DO NOT remove component prop. It will cause some many headeches for me and I guess for many other people. If you dont want, dont support component="select", but allow custom components

@jaredpalmer
Copy link
Owner Author

@klis87 I think you raise some valid points about perf and perhaps we should just deprecate render in v2. Just to recap my thinking....so far in v2, the component prop has been deprecated with a warning. A new as prop has been introduced. component had an identical api to render fn but passed through props. As you pointed out though, you can avoid creating a new function using component instead of render/children which can be a perf boost for large forms. However, the component prop led to a lot of confusion because it yielded different behavior depending on whether the value was string like component="select" or a custom component. This also made it more annoying to use Formik with a lot of UI kits. The new as prop solves the latter problem by keeping the api identical for string and custom components: it just passes onChange, onBlur, name, and value/checked directly through. This makes is easier for folks using something like styled-components/emotion or React Native.

As for optimization concerns, I think you raise very very valid points. Since as prop does not give folks access to the formik bag, it has limited utility aside from direct pass throughs. Perhaps the best path forward would be to:

  • deprecate render
  • keep the component prop, but deprecate string values
  • add the as prop (as it is implement in the rc candidates
  • change docs accordingly

In general, My goal is for people to move away from <Field> over time and use useField instead as it is more ergonomic and much better for building out custom reusable primitives. However, I feel you 100% about perf gains.

Suggested API

as

Spreads onChange, onBlur, name, and value/checked to the input. Can accept string or component values.

<Field as="select">
   {/*...*/}
</Field>

<Field as="textarea" />

// ...

const MyInput = props => <input className="super-input" {...props} />

<Field as={MyInput} />

// ... with sc/emotion

const Input = styled.input`
 /* ... */
`
<Field as={Input} name="thing" />

// or ...

const StyledField = styled(Field)`
/* ... */

<StyledField />

component

const MyInput = ({ field, form, meta, ...props }) => (
    <>
	 <input {...field} />
     {meta.error && meta.touched ? <div>{meta.error}</div> : null}
    </>
)

<Field component={MyInput} name="thing" />

// This will throw a deprecation warning
<Field component="select">
  {/** ... */}
</Field>

@klis87
Copy link
Contributor

klis87 commented Jun 2, 2019

@jaredpalmer Thx for you consideration. I really like your API proposal, I think everyone will be happy.

Regarding useField preference, I think this is a good direction, and generally React community imho will move more and more to hooks. It will take time though. Who knows, maybe next major version of Formik won't even support Field component? But until it does, custom components are nice to be supported.

Generally I very like useField, 2 issues I have with it are:

  • it requires all forms to be rewritten and keeping FIeld for now will give us time to do it
  • It cannot support things like FastField, but I don't know, maybe returning React.memo or PureComponent in our custom input components utilising useField will bring similar performance gains?

@jaredpalmer
Copy link
Owner Author

@klis87 i doubt we will move away from <Field> altogether only because it would be kinda weird to keep <Form> around but not <Field>. It also forces folks to make multiple components (which would ruin "get started in 3 lines of code" value prop). Maybe if we split out formik-dom and formik-native we would move it out of the core formik tho idk. As for fast-field, this is problem with React useContext and there isn't much we can do. We can likely just keep FastField as a class for v2.

@jaredpalmer
Copy link
Owner Author

@crosscompile @klis87 I think we need to move registerField into useField otherwise validate option will not actually trigger.

@jaredpalmer
Copy link
Owner Author

@crosscompile should we look at using forwardRef for Field?

@klis87
Copy link
Contributor

klis87 commented Jun 5, 2019

@jaredpalmer regarding Fast Field I am wondering about it and about using PureComponent in the user land. From what I understand, there is no way to stop Field from being rerendered anyway as Field subscribes to Formik context. Anytime something changes, even when only an input is blurred, all fields will call render because they subscribe to any formik context change, this is how React context works, bypassing any PureComponent optimisations. The only thing which can be done is not to rerender Field children. But isn't that the same as creating custom component with optimized shouldComponentUpdate? Anyway, the biggest performance boost in my opinion could be achieved in the following way:

  • I remember a topic that FastField could work in such a way that it would not update formik context onChange, but onBlur, keeping value in local state until focused. Then, until blur, Formik context would not be updated, hence indeed the rest of unrelated fields would not be rerendered at all. Is there any reason this idea was given up? I think it is a brilliant idea and the best is that we could use useState in useField!

@jaredpalmer so actually you wont give up field level validation? I remember I read that potentially you could leave only form level validation, because field level one was complicated and affected performance

@jaredpalmer
Copy link
Owner Author

@klis87 Yes. That was my first strategy. It mostly worked. There are some edge cases that I never handled properly, like enter key submits. However, I think that could be solved. I think it’s worth exploring again.

@IronSean
Copy link

Can anything be done to allow better type checking of the name property on field? As it stands, there is no validation that a given name actually exists in the form object.

@ltctech
Copy link

ltctech commented Jun 23, 2019

Version: 2.0.1-rc.7

Seems like the <Field> passes a new onBlur and onChange function whenever anything on the form changes. Is this somehow by design? With a form with 10+ fields it's noticeably laggy, especially on IE11. If I want to avoid needlessly re-rendering my custom control passed in via as I have to write my own memo equal function that ignores onBlur and onChange. Have not noticed any side effects yet but there may be some. Is there anyway to avoid this now that <FastField> is gone (now an alias of <Field>)? Also, <FastField> would not check user props, which was bad.

import isEqual from 'react-fast-compare';

//utility function
export function customFieldEqual(prevProps, nextProps) {
    const { onBlur: prevOnBlur, onChange: prevOnChange, ...prevFilteredProps } = prevProps;
    const { onBlur: nextOnBlur, onChange: nextOnChange, ...nextFilteredProps } = nextProps;
    return isEqual(prevFilteredProps, nextFilteredProps);
}

//inside e.g. a react-select or react-datepicker wrapper
export default memo(CustomControl, customFieldEqual);

The same issue exists in useField but I can't find a way to avoid a re-render.

Also deprecating component and replacing it with as does not work for custom controls. I ended up writing a <CustomField> in ES6 that replicates <Field> minus warnings/types but includes meta with isSubmitting and isValidating flags (last two statements statement). Not sure if there is a better way to do it but it works.

export default function CustomField({
    validate,
    name,
    render,
    children,
    as: is,
    component,
    ...props
}) {
    const {
        validate: _validate,
        validationSchema: _validationSchema,
        ...formik
    } = useFormikContext();

    React.useEffect(() => {
        formik.registerField(name, {
            validate: validate,
        });
        return () => {
            formik.unregisterField(name);
        };
    }, [formik, name, validate]);
    const [field, meta] = formik.getFieldProps({ name, ...props });
    const legacyBag = { field, form: formik };

    if (render) {
        return render(legacyBag);
    }

    if (isFunction(children)) {
        return children({ ...legacyBag, meta });
    }

    if (component) {
        // This behavior is backwards compat with earlier Formik 0.9 to 1.x
        if (typeof component === 'string') {
            const { innerRef, ...rest } = props;
            return React.createElement(
                component,
                { ref: innerRef, ...field, ...rest },
                children
            );
        }
        // We don't pass `meta` for backwards compat
        return React.createElement(
            component,
            { field, form: formik, ...props },
            children
        );
    }

    // default to input here so we can check for both `as` and `children` above
    const asElement = is || 'input';

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

    const customMeta = { ...meta, isSubmitting: formik.isSubmitting, isValidating: formik.isValidating };

    return React.createElement(asElement, { ...field, ...props, meta: customMeta }, children);
}

@jaredpalmer
Copy link
Owner Author

jaredpalmer commented Jun 23, 2019 via email

@ltctech
Copy link

ltctech commented Jun 23, 2019

Upon further testing onBlur seems to be OK. It's onChange that's problematic. The issue I think is that state.values is being directly checked as a dependency in executeChange a useCallback. setFieldValue also has state.values as a dependency but it's wrapped in useEventCallback which makes state.values into a useRef. The fix may be to remove state.values dependency from executeChange. Seems to fix the issue when I tried it.

  const executeChange = React.useCallback(
    (eventOrTextValue: string | React.ChangeEvent<any>, maybePath?: string) => {
      // By default, assume that the first argument is a string. This allows us to use
      // handleChange with React Native and React Native Web's onChangeText prop which
      // provides just the value of the input.
      let field = maybePath;
      let val = eventOrTextValue;
      let parsed;
      // If the first argument is not a string though, it has to be a synthetic React Event (or a fake one),
      // so we handle like we would a normal HTML change event.
      if (!isString(eventOrTextValue)) {
        // If we can, persist the event
        // @see https://reactjs.org/docs/events.html#event-pooling
        if ((eventOrTextValue as React.ChangeEvent<any>).persist) {
          (eventOrTextValue as React.ChangeEvent<any>).persist();
        }
        const {
          type,
          name,
          id,
          value,
          checked,
          outerHTML,
          options,
          multiple,
        } = (eventOrTextValue as React.ChangeEvent<any>).target;

        field = maybePath ? maybePath : name ? name : id;
        if (!field && __DEV__) {
          warnAboutMissingIdentifier({
            htmlContent: outerHTML,
            documentationAnchorLink: 'handlechange-e-reactchangeeventany--void',
            handlerName: 'handleChange',
          });
        }
        val = /number|range/.test(type)
          ? ((parsed = parseFloat(value)), isNaN(parsed) ? '' : parsed)
          : /checkbox/.test(type) // checkboxes
          ? getValueForCheckbox(getIn(state.values, field!), checked, value)
          : !!multiple // <select multiple>
          ? getSelectedValues(options)
          : value;
      }

      if (field) {
        // Set form fields by name
        setFieldValue(field, val);
      }
    },
    [setFieldValue, state.values]
);

last line:

    [setFieldValue, state.values]

Should maybe be:

    [setFieldValue]

Edit
Still need to do my own comparison because memo's internal comparison is shallow e.g. my meta property is technically new each time even though it's properties are the same. At least it doesn't require yanking properties out now.:

import isEqual from 'react-fast-compare';

export function customFieldEqual(prevProps, nextProps) {
    return isEqual(prevProps, nextProps);
}

export default memo(CustomControl, customFieldEqual);

I've taken a look at useField and see that it's bound to the entire form state. My React knowledge is not great but I don't see an obvious way to make it only depend on a single field's state.

@klis87
Copy link
Contributor

klis87 commented Jul 3, 2019

@jaredpalmer regarding field optimization, that might be interesting to you in case you didn't hear about it yet gnoff/rfcs#2 that could be another way to achieve much better performance and this will work with hooks too

@jaredpalmer
Copy link
Owner Author

Yes, this would be super sweet

@gnoff
Copy link

gnoff commented Jul 8, 2019

@jaredpalmer I was about to do some rounds looking for lib authors who think they might benefit from useContextSelector. I have a build with it implemented and so we can actually try out libraries with the new API and see how they perform. Would you be interesting in a collab to see if it actually benefits things at some point?

@gnoff
Copy link

gnoff commented Jul 8, 2019

updated RFC PR: reactjs/rfcs#119

@prajavk
Copy link

prajavk commented Jul 24, 2019

@jaredpalmer
If Field render, component are deprecated. Please update Field API examples, if formik going to change ?
Recently, we started using Field render in project. Formik 1.5.8
Still, not clear how Field render handles if null values are initialized. Any solution for it?
<Formik
initialValues={{ name: null, email: "", isEnabled: null }} >

@stale stale bot added the stale label Sep 22, 2019
@dineshrsharma
Copy link

Yes. And you use Field in LoginForm scope ;)

Also, you can compose your FormField component this way

import TextField from '@material-ui/core/TextField';
import React from 'react';

const FormField = props => {
  return (
    <Field {...props}>
      {({ field, form, meta, ...otherProps }) => {
        const error = form.touched[field.name] && form.errors[field.name];
        return (
          <TextField
            {...field}
            {...otherProps}
            error={!!error}
            helperText={error}
          />
        );
      }}
    </Field>
  );
};

const LoginForm = () => (
  <Form>
    <FormField name="email" type="text" label="Email" variant="outlined" />
  </Form>
);

Hi @Andreyco ,
With this approach my form works fine however validationSchema fails with nested objects. There isn't any error which I can look into. It just does nothing.
I tried to make codesandbox but unfortunately facing cross-origin error. Can you help on this?

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

No branches or pull requests