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

redux-form should not write errors to the console explicitly. #3821

Closed
naw opened this issue Feb 6, 2018 · 6 comments
Closed

redux-form should not write errors to the console explicitly. #3821

naw opened this issue Feb 6, 2018 · 6 comments

Comments

@naw
Copy link
Contributor

naw commented Feb 6, 2018

Formerly #3723 was merged to address #3636

The original issue (#3636) was that exceptions thrown in an onSubmitSucceeded handler were being swallowed. The fix in #3723 handled this by explicitly logging an error to the console (i.e. with console.error)

I'd like to suggest that this isn't the correct fix for the problem.

When an error is thrown within a promise, the normal and expected behavior is for the promise to reject. This was occurring, but due to a separate issue (#3303), the promise was later being marked as resolved.

The correct fix to the original problem is to let the error in an onSubmitSucceeded handler bubble up as an unhandled rejection. This has already been solved in #3744, although it wasn't solved at the time that #3636 was opened.

In other words, the fix in #3744 makes the fix in #3723 unnecessary, and also the fix (explicit use of console.error) in #3723 is undesirable. We should not consider exceptions raised in a promise to be errors that should be written to the console -- instead, just let them bubble up as rejected promises.

I'm opening this issue to request that you consider reverting #3723

Thanks!

@naw
Copy link
Contributor Author

naw commented Feb 6, 2018

cc/ @h1b9b @ClementParis016

@naw
Copy link
Contributor Author

naw commented Feb 7, 2018

Note: The current code will console.error not just for errors thrown in onSubmitSucceeded, but for errors thrown in onSubmit also. So supposing you return a promise from onSubmit that rejects with an error that is not a SubmissionError, then the error will be written to the console. My opinion is that it is undesirable for the library to do this by default because a rejected promise returned from onSubmit is not necessarily something we want our application to consider as an error and write to the console.

@ClementParis016
Copy link
Contributor

I totally agree with you @naw, logging the error makes it visible but it doesn't provide any solution to handle it. I would rather like to be able to handle the error myself and report it to an external logging system.

@naw
Copy link
Contributor Author

naw commented Feb 8, 2018

@ClementParis016 To be clear, you can handle the error yourself now by watching for unhandled promise rejections. This was fixed in #3744 and follows normal promise behavior (except for the extra console stuff, of course). Not sure if this solves your particular problem or not.

@erikras
Copy link
Member

erikras commented Mar 2, 2018

Published fix in v7.3.0.

@lock
Copy link

lock bot commented Mar 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Mar 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants