Skip to content

Conversation

benarmston
Copy link

This change makes it possible for client code to distinguish between rejected
and failed promises when attaching then, or catch handlers. With the previous
behaviour it was impossible to make this distinction (at least using the
promise API).

If the old behaviour is desired, a client can add a catch handler which
returns the error before dispatching:

dispatch(myPromise.catch((err) => err))

This change makes it possible for client code to distinguish between rejected
and failed promises when attaching then, or catch handlers.  With the previous
behaviour it was impossible to make this distinction (at least using the
promise API).

If the old behaviour is desired, a client can add a catch handler which
returns the error before dispatching:

```
dispatch(myPromise.catch((err) => err))
```
 - Add returns to expectations including eventually.  Apparently, this is
   required for the test framework to properly test the expectation. See the
   first section of http://chaijs.com/plugins/chai-as-promised.

 - Catch exception thrown now that we're using await with a rejected promise.
@benarmston
Copy link
Author

Bump.

@benarmston
Copy link
Author

@alanrubin have you had any time to look at this? Is not swallowing errors something that you'd like redux-simple-promise to do?

@Jessidhia
Copy link

The documentation also states that "The middleware also returns the original promise", but it certainly does not -- the .then call makes a new Promise.

There is actually an advantage to doing things this way -- you do not have to attach .catch handlers everywhere you dispatch an action to prevent Promise debuggers from flagging an uncaught exception. However, this pattern also does prevent you from attaching a .catch handler if you do want.

The only workaround for now is to save the generated action and add your .catch to the action.payload.promise; but you have to make sure you give the original promise to the dispatch call.

FWIW, the equivalent behavior has been merged on redux-promise: redux-utilities/redux-promise#13

It might be better to just call .then (creating your own Promise) and then returning the original Promise; or changing the contract to resolve with the resolved/rejected action instead of the Promise's result.

@noamokman
Copy link

@alanrubin can this be merged? This is the best package for promises with redux.
The current behavior makes my code look like callbacks 😞

loadUser()
  .then(({error}) => {
    if(error) {
      return;
    }

    redirect();
  });

@alanrubin
Copy link
Owner

First, let me apologise for not touching this project for a long time and thank you for using it. I have time this week and I will look into those proposals and release v2 with the improvements proposed above and in other PRs. I keep you updated here !

@noamokman
Copy link

Awesome! I can help if you want, in the meantime, I'll do a small PR

@alanrubin
Copy link
Owner

After reading the comments here and checking the behaviour in redux-promise, I think throwing is the best option to follow (as implemented by @benarmston), I'm tending to merge that this way. I agree that "The middleware also returns the original promise" is not right as stated by @Kovensky but that seems to be the best behaviour, so I will change the documentation to reflect that. Any thoughts before I do that merge and the change to the documentation?

@noamokman
Copy link

SGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants