jaredpalmer / formik

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

`validate` swallows Exceptions #1329

Open Andreyco opened 5 years ago

Andreyco commented 5 years ago

๐Ÿ› Bug report

Current Behavior

Formik's validate handler swallow all exceptions, silently.

<Formik
  validate={validate}
/>

function validate() {
  throw new Error("Oooops, s*** went wrong");
}

Expected behavior

Error should be displayed in console

Reproducible example

Codesandbox example here https://codesandbox.io/s/xvn1nw30vw

Observations

Solutions proposal

New API

Usage example

import { Formik, FormikValidationException } from 'formik';

function validate(values) {
  const { valid, errors } = someValidator(values);
  if (!valid) {
    throw new FormikValidationException(errors)
  }
}

<Formik validate={validate}>
   {/* children */}
</Formik>

Change existing behavior

Usage example

import { Formik } from 'formik';

function validate(values) {
  const errors = {};
  if (!values.email) {
    errors.email = 'validation message';
  }
  return errors;
}

<Formik validate={validate}>
   {/* children */}
</Formik>

Your environment

Software Version(s)
Formik 1.5.0
React 16.8
f1yn commented 5 years ago

This issue seems to be present for quite some time, also keen on getting some way where it detects an Error instead of a basic validation error object.

As an aside, while I think this throw way of handling validation is clever it seems incredibly unreliable and to be a hack. I'm hoping that future async validation could be be handled as a plain resolve instead of requiring a throw (regardless of whether there is error).

jankalfus commented 5 years ago

I run into the exact same issue today. However, I'm using validationSchema + yup's when. Here's a code snippet:

const validationSchema = yup.object({
    [FORM_NAMES.FILE]: yup
        .mixed()
        .required(VALIDATION_MESSAGES.REQUIRED_FILE),
    [FORM_NAMES.SCHEMA_NAME]: yup.string().when(FORM_NAMES.FILE, {
        is: (file) =>
            file != null &&  // this was missing here and validations were not running when no file was selected
            (file.name.endsWith(PAYLOAD_EXTENSIONS.XML) ||
                file.name.endsWith(PAYLOAD_EXTENSIONS.JSON)),
        then: yup.string().required(VALIDATION_MESSAGES.REQUIRED_FIELD),
        otherwise: yup.string(),
    }),
});

const initialValues = {
    [FORM_NAMES.FILE]: null,
    [FORM_NAMES.SCHEMA_NAME]: "",
};

<Formik initialValues={initialValues} validationSchema={validationSchema} /* etc. */>...</Formik>

When the validation for schema name field missed the null check, it obviously throw an Error. However, Formik swalled it. It was difficult to debug, basically I figured out the exception got swalled by Formik by accident.

I don't think Formik should swallow validation exceptions at all.

Andreyco commented 5 years ago

Yup. I think to resolve instead of to throw is correct approach

CruseCtrl commented 5 years ago

I just ran into this today too. It took ages to work out what was happening, when it should have been obvious from a console error.

The issue seems to be in Formik.tsx at the end of runValidations:

.catch(x => x);

If the promise has rejected, it just returns the error. However, very few things seem to check this return value. One solution could be to just log the error at this time, by replacing the catch block with something like

.catch(error => {
  if (!error.isCanceled) {
    console.error(error);
  }
  return error;
})

Let me know if you think this is the right solution, and I can raise a pull request

Andreyco commented 5 years ago

I think validation exception should be caught in user land and errors should be returned. Formik should not catch anything here.

jannikbuschke commented 5 years ago

Formik also should not use throw/catches/rejections for handling validation errors but inspected the resolved values (similar to synchronized validation, where the returned values indicate validation errors). See https://github.com/jaredpalmer/formik/issues/1141 https://github.com/jaredpalmer/formik/issues/1122 https://github.com/jaredpalmer/formik/issues/966

Andreyco commented 5 years ago

@jaredpalmer I am testing v2-alpha in some of my projects and I see validate expects to throw errors, which is de facto original behaviour. Are you considering a change here? To be precise, returning/expecting resolved errors bag, instead of awaiting Promise rejection?

This could be real pain point when even errors in validation process itself are swallowed (typo, invalid validator, etc.) and hard to discover for many users.

If you are not willing to change this behaviour, at least let's introduce deterministic API, e.g. throwing custom ValidationException with validation errors and detect exception in handler.

jaredpalmer commented 5 years ago

Agree. I think we should fix this in v2.

-- Jared


From: Andrej Badin notifications@github.com Sent: Thursday, May 9, 2019 7:31 PM To: jaredpalmer/formik Cc: Jared Palmer; Mention Subject: Re: [jaredpalmer/formik] validate swallows Exceptions (#1329)

@jaredpalmerhttps://github.com/jaredpalmer I am testing v2-alpha in some of my projects and I see validate expects to throw errors, which is de facto original behaviour. Are you considering a change here? To be precise, returning/expecting resolved errors bag, instead of awaiting Promise rejection?

This could be real pain point when even errors in validation process itself are swallowed (typo, invalid validator, etc.) and hard to discover for many users.

โ€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/jaredpalmer/formik/issues/1329#issuecomment-491102811, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AA67IG6FR6KCYESR4DWN22DPUSX47ANCNFSM4GXI6D3A.

Andreyco commented 5 years ago

@jaredpalmer I added Solutions proposal section in original issue report, please check it out and let's discuss, and/or add other solutions. I am willing to do the work when we agree on preferred solution.

cigzigwon commented 5 years ago

In my case I'm struggling with Formik.props.validate/1 What is the expected behavior of this? I'm passing back the object which clearly has errors but onSubmit is still being called? It's nothing I have control over at this point. The form has errors and still submits.

const validate = async values => {
  let index = tab_instance.index
  let errors = await Promise.all(Object.entries(values).filter(([key, value]) => {
        return key.match(tabs[index]).length
    }).map(([key, value]) => {
        let error = {},
            validator = validators[key]

        if (validator) {
            error[key] = validator(value)
            return error
        }
    })
  )
  errors = errors.reduce((acc, error) => {
    let key = Object.keys(error)[0]
    acc[key] = error[key] 
    return acc
  }, {})
  console.log('errors:', errors)
  return errors
}

And in log

{agent: "Field is required"
agent_borrower_asset: "Field is required"
agent_borrower_product: "Field is required"}
cigzigwon commented 5 years ago

Hey guess what? I think I'm overdoing async stuff lately. :) I forgot that in my case here I'm not talking to an API yet and everything is local but I don't think it will work for me to pass validate asynchronously. I just removed it and it seems to be working now!

cigzigwon commented 5 years ago

Refactored to:

const validate = values => {
  let index = tab_instance.index
  let errors = Object.entries(values).filter(([key, value]) => {
        return key.match(tabs[index]).length
    }).map(([key, value]) => {
        let error = {},
            validator = validators[key]

        if (validator) {
            error[key] = validator(value)
            return error
        }
    }).reduce((acc, error) => {
    let key = Object.keys(error)[0]
    acc[key] = error[key] 
    return acc
  }, {})
  console.log('errors:', errors)
  return errors
}
wontwon commented 5 years ago

Any updates on this? Validation errors are getting really tough to debug and better error reporting would be super helpful. Thanks for your hard work ๐ŸŒฎ

cigzigwon commented 5 years ago

To me it seems like we should at least be getting the console.warn of the actualException. Is this really an issue with Formik or is it a side effect of React Memo? Because in a standalone promise we would surely see the warning with the actual exception. And I would suggest it is not typical to throw errors inside of a Promise function but rather to send it w/reject inside the promise. And yes my NODE_ENV is set to 'development' which is why I ask my question.

wontwon commented 5 years ago

Any updates on this?

cigzigwon commented 4 years ago

If you wrap the validate function code in a try/catch internally and log the error... You will be able to see the original error.

chrisg220 commented 1 year ago

is there an update here on how to eliminate the silent failures?