jaredpalmer / formik

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

FastField: fallback to default shouldComponentUpdate implementation even if custom shouldUpdate implementation returns false #1238

Open Verbon opened 5 years ago

Verbon commented 5 years ago

🚀 Feature request

Current Behavior

Default FastField's shouldComponentUpdate implementation is not triggered, if custom shouldUpdate implementation returns false.

Desired Behavior

If FastField's custom shouldUpdate implementation returns false, then FastField's shouldComponentUpdate default implementation should be triggered.

Suggested Solution

formik/src/FastField.tsx

Current FastField's shouldComponentUpdate implementation:

shouldComponentUpdate(
    props: FastFieldAttributes<Props> & { formik: FormikContext<Values> }
  ) {
    if (this.props.shouldUpdate) {
      return this.props.shouldUpdate(props, this.props);
    } else if (
      getIn(this.props.formik.values, this.props.name) !==
        getIn(props.formik.values, this.props.name) ||
      getIn(this.props.formik.errors, this.props.name) !==
        getIn(props.formik.errors, this.props.name) ||
      getIn(this.props.formik.touched, this.props.name) !==
        getIn(props.formik.touched, this.props.name) ||
      Object.keys(this.props).length !== Object.keys(props).length ||
      this.props.formik.isSubmitting !== props.formik.isSubmitting
    ) {
      return true;
    } else {
      return false;
    }
  }

should be changed to:

shouldComponentUpdate(
    props: FastFieldAttributes<Props> & { formik: FormikContext<Values> }
  ) {
    if (this.props.shouldUpdate && this.props.shouldUpdate(props, this.props)) {
      return true;
    } else if (
      getIn(this.props.formik.values, this.props.name) !==
        getIn(props.formik.values, this.props.name) ||
      getIn(this.props.formik.errors, this.props.name) !==
        getIn(props.formik.errors, this.props.name) ||
      getIn(this.props.formik.touched, this.props.name) !==
        getIn(props.formik.touched, this.props.name) ||
      Object.keys(this.props).length !== Object.keys(props).length ||
      this.props.formik.isSubmitting !== props.formik.isSubmitting
    ) {
      return true;
    } else {
      return false;
    }
  }

Who does this impact? Who is this for?

All the people!

Describe alternatives you've considered

I thought I could achieve my goals by using ref on FastField and manually triggering forceUpdate when I need to, but FastField is a functional component, so ref's are not allowed.

charlax commented 5 years ago

Agree with this - I see no reason you would not want to update on changes in values, errors, touched... etc. for this field.

jaredpalmer commented 5 years ago

Isn't this super confusing though? Is this actually more intuitive?

Verbon commented 5 years ago

@jaredpalmer I don't think this is confusing. I cannot think (at least for now) of any possible use-case, where I would want my FastField to not update when its value/error/touched changes. I think updates in these properties are very important, and skipping updates because of custom shouldUpdate implementation returning false (which may not take these props into account) may lead to bugs and undesired behavior.

If you are very concerned that this would be a breaking and will break a lot of existing code, I may suggest to add a boolean prop shouldUpdateFallbackToDefault (not a very good name, just first thing came to mind), which will configure if default shouldUpdate implementation should or should not be called when custom one returns false.

jaredpalmer commented 5 years ago

Okay makes sense

jaredpalmer commented 5 years ago

I'm cool with a PR

Verbon commented 5 years ago

@jaredpalmer Here's a PR for fixing that behavior - https://github.com/jaredpalmer/formik/pull/1317

supita commented 5 years ago

Hey guys, I just bumped into this issue, we fixed it by basically copy/pasting shouldComponentUpdate into our shouldUpdate function. Is there any updates on this?

nico1510 commented 4 years ago

yes @jaredpalmer somehow this fix got lost when the FastField code was copy&pasted in this commit https://github.com/formium/formik/commit/c93a6d4ad059bb8684f48c2d54524f417add2030. Here is a PR which puts back the fix: https://github.com/formium/formik/pull/2761