jaredpalmer / formik

Build forms in React, without the tears šŸ˜­
https://formik.org
Apache License 2.0
33.98k stars 2.79k forks source link

[v2] Improve Formik.executeChange to accept not only string or event #1667

Open afitiskin opened 5 years ago

afitiskin commented 5 years ago

šŸš€ Feature request

I'm trying to use useField hook to connect formik to custom component:

My custom component uses objects as a value:

const CustomComponent = ({ value, onChange }) => (
  <button onClick={() => onChange({ some: 'value' })}>{value.some}</button>
)

Current Behavior

I'm getting Cannot read property 'type' of undefined error when I connect my custom component to formik like this:

const FormikCustomComponent = (props) => {
  const [field] = useField(props.name);
  return (
    <CustomComponent {...props} {...field} />
  );
};

Error appears when button is clicked (when onChange callback is called). Error thrown because my CustomComponent sends to onChange object instead of string or event. Formik.executeChange checks if passed value is string (it is not) and tries to treat it as an event (and of course fails).

Desired Behavior

Add ability to pass to onChange callback any desired value and use it "as is". executeChange should try to parse value ONLY when EVENT is passed as a value.

Suggested Solution

Currently we check if eventOrTextValue is string, and if it is not a string, we treat it as event. (https://github.com/jaredpalmer/formik/blob/master/src/Formik.tsx#L505) My suggestion is to check if value is EVENT instead of string. If event is passed, then, you need to run code to extract value from the event. In other cases just use passed value "as is" Example solution:

const executeChange = React.useCallback(
    (eventOrTextValue: any | React.ChangeEvent<any>, maybePath?: string) => {
      let field = maybePath;
      let val = eventOrTextValue;
      let parsed;
      // If the first argument is a synthetic React Event or a real Dom Event,
      // we handle like we would a normal HTML change event.
      if (eventOrTextValue && (eventOrTextValue instanceof Event || eventOrTextValue.nativeEvent instanceof Event)) {
        // no changes here ...
      }
      // no changes here...     
    },
    [setFieldValue, state.values]
  );

Possible workarounds

Of course, I'm able to JSON.stringify value before passing it to onChange and JSON.parse it back in component to be able to work with any type of value, but I think it is not a good idea...

Who does this impact? Who is this for?

It will make much easier to connect formik to custom component that works with objects / arrays as a value.

afitiskin commented 5 years ago

I checked source code one more time and found that my suggested solution will not work well, because field.onChange is not bound to field.name.

When event is passed to onChange formik gets field name from event.target.name. When string is passed to onChange formik uses this string as field.name and returns event => executeChange(event, field.name), so we need to call onChange(name)(value) instead of just onChange(value).

So, currently I have 2 suggestions:

  1. Bind field.onChange to field.name passed to useField
  2. Update executeChange to check if passed value is EVENT (see example above)
jaredpalmer commented 5 years ago

@afitiskin yep, i've been thinking of that change for a while. sniffing the event should be straight forward. Want to open a PR?

afitiskin commented 5 years ago

@jaredpalmer May you please describe me the purpose of current handleChange implementation:

const handleChange = React.useCallback(
  (
    eventOrPath: string | React.ChangeEvent<any>
  ): void | ((e: any | React.ChangeEvent<any>) => void) => {
    if (isString(eventOrPath)) {
      return event => executeChange(event, eventOrPath);
    } else {
      executeChange(eventOrPath);
    }
  },
  [executeChange]
);

In types.tsx I also found this:

/** Classic React change handler, keyed by input name */
handleChange(e: React.ChangeEvent<any>): void;
/** Preact-like linkState. Will return a handleChange function.  */
handleChange<T = string | React.ChangeEvent<any>>(
  field: T
): T extends React.ChangeEvent<any>
  ? void
  : ((e: string | React.ChangeEvent<any>) => void);

i found that the function is used only in getFieldProps, but it is also exported from useFormik, so I need a bit more info, how it may be used (since I do not want to brake something accidentally)

afitiskin commented 5 years ago

I do not fully understand why do we need "Preact-like linkState"?

field.onChange(name)(eventOrTextValue) // curried function?

Also, why do we export this function from useFormik?

afitiskin commented 5 years ago

I made a smallest possible PR to fix my issue. I have also few ideas how to improve Field component and useField hook should I open new issue for that?

jaredpalmer commented 5 years ago

FWIW you should be using setFieldValue for your use case. However, I do still want to invert the logic for value checking

afitiskin commented 5 years ago

@jaredpalmer do you mean that I need to use useFormik instead of useField and get/set value of the field manually? Or should I use both useField and useFormik?

afitiskin commented 5 years ago

Using setFieldValue will trigger component render on every formik context change. Since context is huge and it's changed too often we will face a lot of unnecessary re-renders. Current useField implementation has the same problem btw, but it is more easy to fix useField and forget about wrong re-renders.

neiker commented 5 years ago

Related issue: https://github.com/jaredpalmer/formik/issues/1674

afitiskin commented 5 years ago

If somebody is looking for working workaround, here it is. It's an universal formk field component, that works both with native and custom fields:

import React, { useState, useCallback } from 'react';
import PropTypes from 'prop-types';
import { useField, useFormikContext } from 'formik';

const isEvent = (event) => event && (event instanceof Event || event.nativeEvent instanceof Event);

const UniversalFormikField = (props) => {
  const { component: Component, name, ...otherProps } = props;
  const [field] = useField(name);
  const { setFieldValue, setFieldTouched } = useFormikContext();

  const onChange = useCallback((eventOrValue) => {
    if (isEvent(eventOrValue)) {
      field.onChange(eventOrValue);
    } else {
      setFieldValue(field.name, eventOrValue);
    }
  }, [field.name, field.onChange, setFieldValue]);

  const onBlur = useCallback((eventOrValue) => {
    if (isEvent(eventOrValue)) {
      field.onBlur(eventOrValue);
    } else {
      setFieldTouched(field.name, true);
    }
  }, [field.name, field.onBlur, setFieldTouched]);

  return (
    <Component
      {...field}
      {...otherProps}
      onChange={onChange}
      onBlur={onBlur}
    />
  );
};

FormikField.propTypes = {
  name: PropTypes.string.isRequired,
  component: PropTypes.any,
};

FormikField.defaultProps = {
  component: 'input',
};

export default UniversalFormikField;

This component is not optimised and will force re-render on every formik context change, so use it only in small or medium forms.

Usage example:

<Formik {...formikProps}>
  (({ handleSubmit }) => (
    <form onSubmit={handleSubmit}>
      <UniversalFormikField name="email" type="email" />
      <UniversalFormikField name="something" component={MyCustomComponent} />
      <button type="submit">Save</button>
    </form>
  ))
</Formik>
sibelius commented 5 years ago

for me useField('name') should provide the same handler as setFieldValue('name', <value>)