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

v6: Using bluebird in asyncValidate causes warning #1153

Closed
q42jaap opened this issue Jun 13, 2016 · 10 comments
Closed

v6: Using bluebird in asyncValidate causes warning #1153

q42jaap opened this issue Jun 13, 2016 · 10 comments

Comments

@q42jaap
Copy link

q42jaap commented Jun 13, 2016

When using a bluebird promise in asyncValidate, invalidating a field will cause a warning:

Unhandled rejection undefined

This is the line that triggers it:
https://github.com/erikras/redux-form/blob/v6/src/asyncValidation.js#L12

The asyncValidate is in the blur handler, this is the line of code that is ignoring the promise.
https://github.com/erikras/redux-form/blob/v6/src/events/createOnBlur.js#L10

I saw that on master, silencePromise is used to fix this, however in v6 silencePromise has been removed.

The line https://github.com/erikras/redux-form/blob/v6/src/createFieldProps.js#L46
is missing a silencePromise. I think it's still necessary with Bluebird (since it has a detection system for unhandled rejections).

@q42jaap q42jaap changed the title Using bluebird in asyncValidate causes warning v6: Using bluebird in asyncValidate causes warning Jun 13, 2016
@eloytoro
Copy link
Contributor

@eloytoro
Copy link
Contributor

@q42jaap
Copy link
Author

q42jaap commented Jun 13, 2016

That would be a rather devious way to get around
asyncValidate.bind(null, name) not handling the returned promise at the end of the chain.

Point is, that as a library, Promises that are created should always be handled, or they should be able to be handled by user code (like handleSubmit does when returnRejectedSubmitPromise is set to true). Redux-form should not rely on any Promise library's behaviour. Line 46 in createFieldProps lacks handling the promise returned by it's internal asyncValidate function.

The particular rejection from the onBlur point of view is strange, the asyncValidation returned a non-empty error, it's adhering to the api, but along the way, a rejected promise was created (https://github.com/erikras/redux-form/blob/v6/src/asyncValidation.js#L12)

@chrisrittelmeyer
Copy link

chrisrittelmeyer commented Jun 24, 2016

@q42jaap I also had this problem and tracked it to https://github.com/erikras/redux-form/blob/v6/src/asyncValidation.js#L12 which is bubbling up to https://github.com/erikras/redux-form/blob/v6/src/asyncValidation.js#L20

If you add, for instance, .catch(function(e) {}); to the end of that line, the warning stops. That's probably not the best way to handle it, of course...

@eloytoro
Copy link
Contributor

Its my opinion that its ok to expose promises that could be rejected for the client to handle them.
If the library were to catch all the errors the client wouldn't be able to introduce its own handle logic.
And by reading the source code it seems to be doing exactly that, but if the rejection isn't carrying an error payload that should be fixed.
If you define async validation that could fail its in your best interest to define some logic to handle that.
If you dont like the way promises throw errors when unhandled theres a whole thread in the node repo discussing this.
@chrisrittelmeyer that clears the issue because thats the async equivalent of try/catching something that throws an error, but leaving the catch part empty.

@chmanie
Copy link

chmanie commented Dec 5, 2016

I ran into this problem and I agree with you @eloytoro except that I encountered a specific problem that is unsolvable the way things are implemented currently:

Use case

  • User fills out the form, which has just one field that includes async validation.
  • User does NOT blur this field but clicks on the submit button instead (which is possible!)
  • On submit async validation is being run and throws an error. But this time it is run in a different context. It is throwing an error which is going to be unhandled and causes the script to reside in an undefined state. The next time you submit the form it won't work.

You can test this with the examples (I'm using Chrome 54, also tried Firefox).

http://redux-form.com/6.2.0/examples/asyncValidation/

  • Type in the password first (!)
  • Then choose a username that's taken (e.g. john). DO NOT BLUR THIS FIELD, dismiss autocomplete with ESC if necessary
  • Immediately click on Sign up
  • An uncaught error will appear in the console
  • Now try any valid username
  • Signup won't work anymore no matter how often you try (you have to refresh the page to make it work again)

You might say, well, in this case it's not a big problem because it's very specific. But it looks quite differently if you have only one form field.

EDIT:

I have identified this line to be the cause of all trouble:

https://github.com/erikras/redux-form/blob/master/src/handleSubmit.js#L82

(it won't even work when setting a onunhandledrejection listener on window, while preventing the default action)

@danielrob
Copy link
Collaborator

@chmanie I followed your reproduction steps successfully on older versions to produce an uncaught promise exception (thank you for these!), but unsuccessfully on 7.0.3 🎉.

Would those in this thread be willing to confirm / provide further investigation if needed to clarify the status of this issue?

@chmanie
Copy link

chmanie commented Jul 27, 2017

You could reproduce it with the async validation example; I tried with http://redux-form.com/7.0.3/examples/asyncValidation/ and it did not occur. I'm willing to say that it's resolved now.

@gustavohenke
Copy link
Collaborator

Cool, thanks!

@lock
Copy link

lock bot commented Jul 27, 2018

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 Jul 27, 2018
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

6 participants