Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Synchronous errors from onSubmit are ignored in v3 #819

Closed
radekmie opened this issue Oct 24, 2020 · 2 comments
Closed

Synchronous errors from onSubmit are ignored in v3 #819

radekmie opened this issue Oct 24, 2020 · 2 comments
Assignees
Labels
Type: Bug Bug reports and their fixes
Milestone

Comments

@radekmie
Copy link
Contributor

As found it while working on #812, there's a problem with errors from onSubmit. These errors are treated as results and not actual problems. The problematic code is here.

onSubmit={model => {
  const errorOrNull = synchronousValidation(model);

   // Does not work but should.
  return errorOrNull;

  // Works as expected. 
  return Promise.reject(errorOrNull);
}}
@radekmie radekmie added the Type: Bug Bug reports and their fixes label Oct 24, 2020
@radekmie radekmie added this to the v3.0 milestone Oct 24, 2020
@radekmie radekmie self-assigned this Oct 24, 2020
@radekmie
Copy link
Contributor Author

Some update: while working on that, I've come to the conclusion that synchronous errors from onSubmit are problematic. Returning them may be error-prone, as people may expect it to work in the same way as returning a value from asynchronous submission. Allowing throwing synchronous errors in onSubmit is problematic as well, as it may hide actual problems. Another option would be to make it work as follows:

onSubmit={model => { return value; }} // [A] This value is accessible in `form.submit()` Promise. [B] This value is ignored.
onSubmit={model => { throw error; }} // This is an error, `onSubmit` cannot throw an error.
onSubmit={model => { return Promise.resolve(value); }} // This value is accessible in `form.submit()` Promise.
onSubmit={model => { return Promise.reject(error); }} // This error is treated as a validation error, being accessible in the form.

I think that is the best, as most onSubmit functions are asynchronous. Another, maybe easier to understand option ([B]), would be to force onSubmit: (model: Model) => void | Promise<any>, so ignore any synchronous result.

@Monteth
Copy link
Member

Monteth commented Oct 30, 2020

I would opt for the 'B' as it's simpler and more consistent. I believe wrapping errors to Promise in sync scenarios won't be harmful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Bug reports and their fixes
Projects
Archived in project
Development

No branches or pull requests

2 participants