createAsyncThunk return fulfilled/rejected action instead of re-…

[phryneas]

This is an alternative suggestion to #359.

I've created a codesandbox that references the current alpha and implements a small data-fetching workflow.
The problem that becomes apparent is that, if there is an error within fetch and that error is not caught from dispatch, CRA in Dev Mode displays the red overlay of "something went terribly wrong":

This happens during an event handler as well as during useEffect.

In production, this would probably not happen, but realisticly, devs would react to this by wrapping every dispatch call in an async function with try/catch or append a .catch() to it.
Doing the first thing is quite complicated, because you can't just change an effect to an async function (because that would return a promise), so you come up with something like

  React.useEffect(() => {
    async function doFetch() {
      try {
        dispatch(fetchDataThunk({}));
      } catch {
        // do nothing
      }
    }
    if (state === "initial") {
      doFetch();
    }
  }, [state, dispatch]);

for something that would otherwise just have been

  React.useEffect(() => {
    if (state === "initial") {
        dispatch(fetchDataThunk({}));
    }
  }, [state, dispatch]);

So this makes stuff much more complicated - also, event handlers that would just have been () => dispatch(fetchDataThunk({})) now become either () => dispatch(fetchDataThunk({})).catch() or async () => { try {dispatch(fetchDataThunk({}))} catch {} }.

What I want to say: avoiding this gets really messy.

So this is my suggestion of another solution:
The thunk would not return the promise return value in this case, but the "last dispatched action", which means either the fulfilled or the rejected action. That way, the error can be accessed from React, but not everyone is forced to handle it correctly.

[netlify]

Deploy preview for redux-starter-kit-docs ready!

Built with commit 78acd7d

https://deploy-preview-361--redux-starter-kit-docs.netlify.com

[phryneas]

Oh, just to bring your attention to this: before, pending, rejected and fulfilled where any-typed, the approach with Object.assign is the easiest way to preserve the types here.

[markerikson]

Yeah, a user was alpha-testing createAsyncThunk last night and pointed this out, and it's exactly what I was also wondering might be a point of concern.

I've semi-frequently seen folks try to chain off thunks in components, but they've generally only cared about the success case.

Having to check whether the promise return value is a failure action is awkward, but probably the best alternative here.

[markerikson]

hah, you forgot to update the public API file :) I've only done that FOR EVERY ATTEMPT TO PUBLISH AN ALPHA SO FAR!

[phryneas]

Yeah, it's not perfect, but I think it can still be written pretty compact using the .match method:
dispatch(async(3)).then(result => async.fulfilled.match(result) && /* do something */console.log(result))
We'll just have to document that.

We could even add a helper that would re-introduce a throw for people that want it, so
dispatch(async(3)).then(async.throwErrorsFromPromise).then(/*...*).catch(/*...*/)
could be a possibility.

[phryneas]

hah, you forgot to update the public API file :) I've only done that FOR EVERY ATTEMPT TO PUBLISH AN ALPHA SO FAR!

Yap :D
I even created it and force-pushed an amended commit where I forgot to add the change before amending -.-

[markerikson]

Sure, wanna put that throw helper in real fast?

[phryneas]

Yap, why not :)

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

[phryneas]

Okay, it's in there. If you have a better idea, feel free to change that method name :D

[markerikson]

How about rejectIfError?

dispatch(fetchStuff())
    .then(rejectIfError)
    .catch(handleError)

etc

[phryneas]

How about rejectIfError?

dispatch(fetchStuff())
    .then(rejectIfError)
    .catch(handleError)

etc

Hmm. Both names don't really reflect the other side of it, that the return value would also be unwrapped. 🤔

unwrapAndRejectIfError would reflect what it really does, but is a bit long.. splitPromise makes sense if you know what it does... normalizePromise likewise, but from the alternatives I can come up I think it's the best-sounding.... not really sure there. 🤷‍♂️