jannikbuschke / formik-antd

Simple declarative bindings for Ant Design and Formik.
https://codesandbox.io/s/github/jannikbuschke/formik-antd-example
MIT License
589 stars 81 forks source link

DatePicker not respecting format strings #151

Open karlshea opened 4 years ago

karlshea commented 4 years ago

It looks like the format string in DatePicker isn't respected when setting form values. I have a picker for a date-only field (passing YYYY-MM-DD into initial form values, and expecting one back), but the value in the submit handler is a full ISO string. This is leading to timezone issues where there shouldn't be any tz information at all.

It also can't be solved with an onChange for the field, because both the moment object and the date string are still correct when that's run. I traced it back to this package's field, which sets the form's field value by calling toISOString() on the moment date object.

Before I make a PR, I'd like some feedback on my solution (or if that's breaking this can be an example for anyone else that's running into the same issue):

const DatePicker = ({
  name,
  validate,
  onChange,
  fast,
  ...restProps
}: DatePickerProps) => (
  <Field name={name} validate={validate} fast={fast}>
    {({
      field: { value },
      form: { setFieldValue, setFieldTouched },
    }: FieldProps) => (
      <$DatePicker
        value={value ? moment(value) : undefined}
        onChange={(date, dateString) => {
// CHANGE START
          const format = date ? date.creationData().format : null;
          setFieldValue(name, date ? (format ? date.format(format.toString()) : date.toISOString()) : null);
// CHANGE END
          setFieldTouched(name, true, false);
          onChange && onChange(date, dateString);
        }}
        {...restProps}
      />
    )}
  </Field>
);

The date moment object contains the format string that antd is using in the picker to format the date, so if it's there I can format the date using the same string on the way out, and it falls back to toISOString() if it's not found.

jannikbuschke commented 4 years ago

Good old time issues :)

Mh, I'm not so sure about your solution. I probably need some time to think about the problem and the solution. Also ant d now supports other datetime libraries (like dayjs), something that maybe should also be considered.

Wouldnt it make sense for you to adjust your datestring before giving it to formik? So internally the component handles a "iso" date, maybe as a UTC+0 date. when you submit you just remove this information again.

// I know, this is not ideal, but I also know that dealing with time stuff is inherently tricky. Your solution might make your use case a bit easier, but the implementation now is trickier to understand and to reason about.

karlshea commented 4 years ago

Wouldnt it make sense for you to adjust your datestring before giving it to formik?

Isn't that what this is doing? The onChange handler here is the only place to access the antd component's moment instance before it gets passed to formik.

It would definitely be better if internally the component used a UTC date, because that would make it easier to do tz adjustments afterwards (maybe?), but it seems like either way there should be a better way to manipulate the date before it hits formik state. Because right now a date can't make a round-trip without being changed if your browser isn't at UTC+0.

Maybe there should be another callback between onChange and setFieldValue?

You're right, dealing with date/time is always a pain! Thanks for taking a look at this.

damonsmith commented 3 years ago

To me it looks like the antd datepicker returns both the date object and the formatted string, but formik-antd takes only the date object, does an toISOString() on it and then always returns that as the value, but that introduces a whole bunch of time and timezone related issues that I would have to solve by parsing and re-formatting the date object.

I'm thinking it would be an option to add a property to the formik-antd to choose whether the form field is the date object or the formatted string. Shall I do a PR for that?

karlshea commented 3 years ago

In the meantime I've moved away from Antd, so I'm not invested in the outcome of this issue. It can be closed if no one else is running into this problem.

damonsmith commented 3 years ago

I've raised a PR for this: https://github.com/jannikbuschke/formik-antd/pull/175