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

Unhandled promise rejection #75

Closed
pathsny opened this issue May 19, 2016 · 12 comments
Closed

Unhandled promise rejection #75

pathsny opened this issue May 19, 2016 · 12 comments
Labels
question Issues blocked by a question

Comments

@pathsny
Copy link

pathsny commented May 19, 2016

I have a question about what happens when the payload is a promise that is rejected. I see that the middleware correctly dispatches an event with the _REJECTED suffix.

However, I also see a log entry in the console about an unhandled rejection. This is coming from es6.promise. I dont understand why this is considered unhandled, considering that the middleware has dealt with the promise rejection. Is there something I'm supposed to be doing here?

@tomatau
Copy link
Collaborator

tomatau commented May 19, 2016

Which version are you using?

@pathsny
Copy link
Author

pathsny commented May 19, 2016

3.0.0

@trollknurr
Copy link

+1 for answer

@pburtchaell
Copy link
Owner

pburtchaell commented May 27, 2016

@pathsny @trollknurr The middleware dispatches a rejected action, but this does not actually handle the rejected promise. My suggestion would be to either (a) create a second middleware, following the promise middleware, that globally handles all errors/rejected promises; or (b) handle the rejected promise in the action creator, if you are using thunk middleware.

// Example with thunk middleware
export function fooActionCreator() {
  return dispatch => dispatch({
    type: 'FOO_ACTION',
    payload: Promise.reject()
  }).catch(...);
}

The reason this middleware does not handle the rejection is because it takes responsibility for dispatching actions only. It is designed to avoid swallowing errors.

Please let me know if this answers your question! Also, sorry for the delay, but I was engaged with finals at school, relocating to CA for a new job and finishing up other work commitments.

@pburtchaell pburtchaell added the help wanted Issues the project could use your help on label May 27, 2016
@pburtchaell pburtchaell added question Issues blocked by a question and removed help wanted Issues the project could use your help on labels May 27, 2016
@pathsny
Copy link
Author

pathsny commented May 30, 2016

I respectfully disagree with the design decision.

  1. The middleware isnt really swallowing the error since you are dispatching a REJECTED action allowing the reducer to handle the error, so forcing users to add a catch to every promise is a bit silly
  2. If users want to catch the rejected promise, the can still do so by catching the promise and doing what's necessary and then rethrowing so that it can be handled by the middleware.

If you really believe that it's necessary to provide users the option to be able to catch a rejected promise, can I suggest adding an option at middleware choice time to turn this off globally?

@pburtchaell
Copy link
Owner

pburtchaell commented May 30, 2016

forcing users to add a catch to every promise is a bit silly

You are right, it is not a practical solution. The more practical solution is to build global handling into a middleware. In my opinion, however, building global handling built into the promise middleware doesn't make sense. Instead, I would refer to my suggestion in the previous comment: "create a second middleware, following the promise middleware, that globally handles all errors/rejected promises."

If you really believe that it's necessary to provide users the option to be able to catch a rejected promise, can I suggest adding an option at middleware choice time to turn this off globally?

I'm open to exploring changes the middleware, but I don't have time in my schedule now. Addtionally, I don't believe the proposed change makes sense, so I don't want to invest time into it. If you have time and can make some examples, we can go from there! 🔢

  1. Create an example explicitly demonstrating the current behavior you wish to avoid
  2. Create an example with the change to the middleware to prevent the behavior

This would give me a better understanding of how you wish to change the middleware.

@tomatau
Copy link
Collaborator

tomatau commented May 30, 2016

@pathsny -- what harm comes from an unhandled promise rejection error? The appropriate actions are updated so the app state can react and remain resilient... but what problem would an error in the console cause? I think the errors being propagated are actually useful for debugging -- rather than the promise middleware eating them up (like it used to -- which was my fault)

@pathsny
Copy link
Author

pathsny commented May 30, 2016

The harm is merely annoyance in development. Unnecessary log spew tends to
lead to blindness towards actual unhandled errors. I tend to think of these
errors as handled because the reducer is informed about them and updates
the state appropriately
On May 30, 2016 12:36, "Thomas" notifications@github.com wrote:

@pathsny https://github.com/pathsny -- what harm comes from an
unhandled promise rejection error? The appropriate actions are updated so
the app state can react and remain resilient... but what problem would an
error in the console cause? I think the errors being propagated are
actually useful for debugging -- rather than the promise middleware eating
them up (like it used to)


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#75 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAHsto2LXa-oswjLveEDahrld8z4-JMvks5qGzwzgaJpZM4Ih6K_
.

@pburtchaell
Copy link
Owner

@pathsny I would suggest implementing the global middleware if you want to avoid the annoyance. I don't find it very productive to consider this an actual issue with the project though.

@pathsny
Copy link
Author

pathsny commented May 30, 2016

I understand. Thanks, I'll look into that
On May 30, 2016 13:35, "Patrick Burtchaell" notifications@github.com
wrote:

@pathsny https://github.com/pathsny I would suggest implementing the
global middleware if you want to avoid the annoyance. I don't find it very
productive to consider this an actual issue with the project though.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#75 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAHstmLsd2wKKCKTkREj7mu42-O_QLe3ks5qG0oSgaJpZM4Ih6K_
.

@DoubleU23
Copy link

DoubleU23 commented Sep 16, 2016

i also don't understand why this behaviour was changed,
because it isn't really an unhandled exception when it's handled by the middleware which pushes it into the actions returnvalue where you can handle it if you need to (the _ERROR reducer may be enough)

i had to fallback to v2.2.3 where the thrown errors (rejected promises) are handled as expected:
if the promise is rejected, the middleware dispatches the _ERROR action with
error: true
payload: the thrown error/rejected value

IMHO thats the way to go

Repository owner locked and limited conversation to collaborators Sep 16, 2016
@pburtchaell
Copy link
Owner

@DoubleU23 The current version, as do all versions of the middleware, dispatch the rejected action with error: true and the error as the payload. The middleware is not responsible for catching the rejected value. That results in swallowed errors and is not a good practice.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
question Issues blocked by a question
Projects
None yet
Development

No branches or pull requests

5 participants