reduxjs / redux-toolkit

The official, opinionated, batteries-included toolset for efficient Redux development
https://redux-toolkit.js.org
MIT License
10.7k stars 1.17k forks source link

Abort createAsyncThunk Uncaught (in promise) #567

Closed mfabre42 closed 4 years ago

mfabre42 commented 4 years ago

I found a bug (or a feature...), the promise.abort console a Uncaught (in promise) when the promise wasn't actually called due to condition in createAsyncThunk.

    // same probleme with useEffect
    useLayoutEffect(() =>
    {
        const promise = dispatch(getData());

        return () =>
        {
            promise.abort('Unmount on pending');
        };
    }, [dispatch]);
export const getData = createAsyncThunk<Data, undefined, { state: RootState }>(
    'apiData/getData',
    async () => DataServices.getData(),
    {
        condition: (_, { getState }) =>
        {
            const { data } = getState();
            return data.getDataStatus !== Status.Fulfilled;
        },
    },
);

If my condition return false and then we don't do the cycle of createAsyncThunk, my promise exist and do the abort, but the abort console redux-toolkit.esm.js:1345 Uncaught (in promise) {name: "AbortError", message: "Unmount on pending"}.

I have other way to handle this but this way is cleaner. Is it "normal" and I should just handle myself the condition and unmount in the useEffect or is it a bug and the abort should not be triggered or console the Uncaught ?

phryneas commented 4 years ago

Hah, you found a legit bug. Nothing in combination with createAsyncThunk should ever throw anything by default.

From the back of my head, it seems that we would need something like

// in src/createAsyncThunk.ts:

          if (
            options &&
            options.condition &&
            options.condition(arg, { getState, extra }) === false
          ) {
+            abortedPromise.catch()
            throw {
              name: 'ConditionError',
              message: 'Aborted due to condition callback returning false.'
            }
          }

Would you be willing to open up a PR for this, with this fix and a test case for this situation?

I can do so, too, but I have quite the backlog, so it would take some time - I'd be happy for someone to take over this one.

mfabre42 commented 4 years ago

Hello, since I have never done that for any project on github, I'll see if I have the time to do it today cause I have to check how to do it.

Thanks a lot :)

Update : I'm not able to do it sadly. It would take me way too much time for now. But yeah, I should do more open source coding so I will be able to take a moment do to these kind of things in the future.

msutkowski commented 4 years ago

@mfabre42 This helped me a lot initially, and hopefully, it'll help you out as well! It's easier than you think: https://www.gun.io/blog/how-to-github-fork-branch-and-pull-request

alextrastero commented 3 years ago

Sorry for waking up the dead but I'm getting an error in the console (plus in FF it's triggering the ErrorBoundary during development). Using @reduxjs/toolkit@1.6.1

Uncaught (in promise) ConditionError: Aborted due to condition callback returning false.

slice.ts

export const fetchConfigAC = createAsyncThunk(
  `${BASE}/fetch`, myServiceFetchConfig, {
    condition: (_, { getState }: any) => {
      return getState().config.status === Status.FULFILLED;
    }
  }
)

Is it expected to log this issue in console?

Bonus where to get type of second argument of condition func?

phryneas commented 3 years ago

Aborting an asyncThunk will result in a rejected action to be returned from the dispatch call. If you use .unwrap or unwrapResult, that will cause a throw in the case of a rejected action.

When using .unwrap or unwrapResult you always have to use try..catch or .catch, no matter what, or you will leak errors. This applies always when you work with promises.

I might have misread that in a wrong context. Could you please provide a reproduction?

alextrastero commented 3 years ago

Sure, https://codesandbox.io/s/reactredux-forked-j56yb

edit as I wrote the sandbox I realised that the error is not with this library, a condition returning false throws an error which is valid, it's up to me to handle each error

phryneas commented 3 years ago

Okay, my initial interpretation was apparently right and this has nothing to to with the context of this issue, so here again without the strikethrough:

Aborting an asyncThunk will result in a rejected action to be returned from the dispatch call. If you use .unwrap or unwrapResult, that will cause a throw in the case of a rejected action.

When using .unwrap or unwrapResult you always have to use try..catch or .catch, no matter what, or you will leak errors. This applies always when you work with promises.

alextrastero commented 3 years ago

Thanks for the clarification and your time @phryneas

towry commented 2 years ago

I am using the 1.8.2 version, and keep getting this Aborted due to condition callback returning false. error and it gets shown to the user by the toast in the catch branch.

I thought if the condition returned false, the thunk will not be run or will be aborted,

After a thunk has been cancelled this way, it will dispatch (and return) a "thunkName/rejected" action with an AbortError on the error property. The thunk will not dispatch any further actions.

So I filtered out the Abort error, should I filter out the condition error in this way too, is there a property on Error to show it's a condition aborted error?

alextrastero commented 2 years ago

@towry I have something like so in place:

const conditionError = 'ConditionError';

export const handleError = (err: SerializedError) => {
  if (err.name !== conditionError) {
    notification.error({ // external lib method
      message: err.name,
      description: err.message,
    })
  }
}