Add optional thunk configuration object

[rjstires]

I sometimes find it useful to catch and respond to errors when dispatching Thunk. Maybe I'm thinking about this incorrectly? I suppose you could argue that createAsyncThunk just isn't the tool for this use case, but the effort to accomodate the feature was minimal and I think it's useful.

This PR introduces using a config object, instead of a string, for the action. The config objects only property currently is whether or not to rethrow an error. A string type can still be used for simple use cases.

• User can specify a simple string for type or a config object.
• Config can choose to rethrow an error.
• Other possible use cases;
  • Prime result function to transform result before return.
  • Prime error function to transform error before return.

[netlify]

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

Built with commit 4d1daaa

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

[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.

Latest deployment of this branch, based on commit 4d1daaa:

Sandbox	Source
proud-fire-53tj0	Configuration
damp-lake-4cix4	Configuration
heuristic-bohr-is33p	Configuration

[markerikson]

I'm not sure I follow the intended usage here. What would an actual in-app usage example of catching an error look like, and where would that code be?

[rjstires]

Maybe a bit contrived but,

const someThunk = createAsyncThunk({ type: 'whatever', rethrow: true }, async () => { throw new Error(`Nope.`); });

// MyComponent.tsx
const MyComponent = () => {
  const dispatch = useDispatch();

  async function handleClick() {
    try {
      await dispatch(someThunk());
    } catch (error) {
      toast(`Whoops!`)
    }
  }

  return (
    <button onClick={handleClick}>Click Me!</button>
  );
}

/** versus */

const someThunk = createAsyncThunk('whatever', async () => { throw new Error(`Nope.`); });

const MyComponent = () => {
  const dispatch = useDispatch();
  const error = useSelector((state) => state.error);
  const errorRef = useRef(error);

  useEffect(() => {
    if (error && error !== errorRef.current) {
      toast(`Whoops!`);
    }
    errorRef.current = error;
  }, [toast, error, errorRef]);

  function handleClick() {
    dispatch(someThunk());
  }

  return (
    <button onClick={handleClick}>Click Me!</button>
  );
}

[phryneas]

The "versus" can be reduced to a reusable custom hook, so in the end it's something like

useToastOnError(substateSelector)

which would describe an effect nicely, and can be used in a different component than the dispatch call. Having it rethrown introduces a tight coupling.

So I don't really see the appeal for this. It also carries the risk of people defining the rethrow at one point and forget to use it elsewhere - with the result of uncaught promise exceptions.

[rjstires]

The "versus" can be reduced to a reusable custom hook, so in the end it's something like

useToastOnError(substateSelector)

which would describe an effect nicely, and can be used in a different component than the dispatch call.

So I don't really see the appeal for this. It also carries the risk of people defining the rethrow at one point and forget to use it elsewhere - with the result of uncaught promise exceptions.

I can't disagree that it's a good use-case for a hook. Maybe I'm still not fully in the hooks mindset. Feel free to close if it's not bringing value or alter if you have other use-cases for a config object over a string.

[markerikson]

I can see a few different arguments either way:

• On the one hand, by using createAsyncThunk, you've already opted into its restricted default way of doing things, and the default behavior is just "dispatch this action on rejection"
• On the other hand, chaining logic in a component off of promises returned from a thunk is a well-known use case, and by catching the error in the thunk, the component would never see a rejected promise.
• On the gripping hand, I don't like adding more config options to this API, including changing the first argument to an object
• And on the, uh, fourth hand (?), you can always define your promise callback in a way that you catch original errors yourself and then transform them appropriately before the result promise gets returned

If anything, I'd be inclined to just have the catch clause rethrow the promise unconditionally.

Meanwhile, I'm also not convinced that we actually need a "finally" action in here either. Thoughts on that one?

[phryneas]

If anything, I'd be inclined to just have the catch clause rethrow the promise unconditionally.

I'm not really a fan of uncaught promises if we can avoid that. We could return an object { result: null, error: Error } from the thunk if we really wanted that, though.

Meanwhile, I'm also not convinced that we actually need a "finally" action in here either. Thoughts on that one?

It might be useful for someone to track all running requests or something of the likes without duplicating the logic to two reducers - but I don't really have an opinion on that one.

[rjstires]

If anything, I'd be inclined to just have the catch clause rethrow the promise unconditionally.

I'm not really a fan of uncaught promises if we can avoid that. We could return an object { result: null, error: Error } from the thunk if we really wanted that, though.

I've never been a fan of "successful errors" but I can appreciate what you're trying to accomplish here. I've fought this battle before and lost.

Meanwhile, I'm also not convinced that we actually need a "finally" action in here either. Thoughts on that one?

It might be useful for someone to track all running requests or something of the likes without duplicating the logic to two reducers - but I don't really have an opinion on that one.

I haven't experienced a use-case that has required using the finally block of a try/catch/finally. I just foresee a lot of people of people "turning loading off" in finally and causing needless re-renders. But that's kind of on them since you're just matching the try/catch/finally pattern.

[phryneas]

One thing to add generally: I'm not a fan of any async logic in a react component, as it will lead to a lot of "setState called after component unmounted" and similar errors down the line and is always prone to memory leaks etc.
I can remember a lot of discussions around the promise that react-async exposes.

So generally, my vote would be against exposing the error via the promise either way to not encourage the use of it.

PS: of course, I love async logic in react components. I'd rather want people to write good hooks with unmount-handling etc around them and not use it in inline event handlers, which will happen here.

[rjstires]

This is some good food for thought. I appreciate the conversation. 👍 Holler if you need any grunt work. Im around.

[phryneas]

I guess we close this now as we have something similar in the current alpha.