onSubmit promise rejection is not propagated

[JoseLion]

🐛 Bug report

Current Behavior

When a promise is returned in the onSubmit function and this promise is rejected, the rejection is not propagated. It seems the promise rejection is caught in the handleSubmit implementation (here) to just print a warning to the console.

This behavior prevent users from catching unhandled rejections of submitted forms in a general way, such as window.addEventListener("unhandledrejection", ...). The only workaround would be to add a .catch(..) on the promise of every onSubmit, which will cause the code to be very repetitive and hard to maintain.

It is possible to propagate the rejection of the promise in the handleSubmit implementation without affecting anything else? If so, I'll be happy to open a PR with the change 🙂

Expected behavior

The rejection of a promise on the onSubmit function is propagated so it could be handled by a global handler such as window.addEventListener("unhandledrejection", ...)

Reproducible example

https://codesandbox.io/s/formik-codesandbox-template-forked-63xtg?file=/index.js

Suggested solution(s)

Remove the catch block in the handleSubmit implementation

Additional context

Your environment

Software	Version(s)
Formik	2.1.5
React	16.13.1
TypeScript	4.0.3
Browser	Google Chrome
npm/Yarn	Yarn 1.22.4
Operating System	Windows 10 Pro

[fhollermayer]

'(unhandled) promise rejection is not propagated' sounds like a bad thing if you put it that way, but actually a 'unhandledrejection' listener should only be the last line of defense against an application crash. So a precautious catch is generally a good thing.

The implementation of onSubmit is in user-land. So one could argue, that error handling should be left to the developer. But the invocation of onSubmit is Formik's concern. And so an error coming out of that call has to be taken care of by Formik.

If you want to handle errors thrown by your implementation do it in your implementation.

[JoseLion]

I didn't mean to make it sound that bad, sorry for that. I agree that the unhandledrejection listener could be the last line of defense, but it's also not good to add the same precautious catch block in every onSubmit of every form. It's not only hard to maintain but also goes against the DRY principle. I understand the invocation of onSubmit is Formik's concern. Still, Formik is actually invoking the user's code, so catching all errors in their place and sending only a warning seems not ideal to me. 🤔

IMHO, I think developers should handle any errors downstream (using middleware, listeners, etc.). Hiding errors from the users seem like an issue to me. It actually made me spent hours troubleshooting until I figured the promise in the onSubmit was actually rejected and then resolved with a generic message warning in its place. 😓

BTW, I used patch-package in my project to remove the catch block, and it looks like everything is working just fine 🙂

+ submitForm();
- submitForm().catch(reason => {
-   console.warn(
-     `Warning: An unhandled error was caught from submitForm()`,
-     reason
-   );
});

Cheers!

[pschinis]

I'm with @JoseLion on this one. IMO failing silently like this and thus disallowing any kind of centralized error handling/logging is kind of a major issue. If I have forms that just aren't working for one unforeseen reason or another now I just won't know about it. And also more generally just failing silently like this is questionably better than letting the app crash... I mean you're pretty much defaulting to having a broken/hanging interface or letting the user go on thinking whatever action they just took succeeded when in fact it did not. Maybe sometimes that's better than the alternative of just crashing but in many cases it's better for the user to just know the thing is broken

[github-actions[bot]]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 60 days

[HansAarneLiblik]

This also affects our usage

const doSubmitting = (values) => axios.post(.., values);  // Rejected promise errors dissapear
const doSubmitting = (values) => { axios.post(..., values); };  // Rejected promise errors are propagated

<Formik
  onSubmit={doSubmitting}
  ...
  />

[github-actions[bot]]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 60 days

[JoseLion]

I opened PR #3052 that removes the .catch(..) in the promise of the handleSubmit that should solve the issue without affecting anything else 🙂

[github-actions[bot]]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 60 days

[github-actions[bot]]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 60 days

[github-actions[bot]]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 60 days

[github-actions[bot]]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 60 days

[hixus]

I fully agree that the catch should be removed as it's not really handling the error. For example; we can't see submit errors in Sentry unless we always remember to add our own try catch to onSubmit function.

Luckily noticed this on pre-launch stage.

[jesperborgstrup]

FWIW, I have mitigated the issue by invoking submitForm instead of handleSubmit in our submit button onClick handlers. (handleSubmit in essence just calls submitForm and stops any errors from propagating)

Furthermore, this simple rule for ESLint ensures that any handleSubmit calls don't sneak back into the codebase:

{
  // ...
  rules: {
    'no-restricted-syntax': [
      'error',
      {
        message:
          'Use Formik submitForm instead of handleSubmit. handleSubmit hides errors from Sentry: https://github.com/jaredpalmer/formik/issues/2758',
        selector: 'Identifier[name="handleSubmit"]',
      },
    ],
  },
}

[avasuro]

@jesperborgstrup Thank you, that solution helped me a lot!

Totally agree that catch should be removed. Preventing the error from being thrown doesn't adds any value, but doesn't allow to use global error catcher (e.g. to send error reports in production app to the reporting system).