Subtle footgun in RTKQuery onQueryStarted queryFulfilled usage.

[joel1st]

This issue is a combination of docs & api design. The document in question: https://redux-toolkit.js.org/rtk-query/usage/manual-cache-updates

Currently queryFulfilled returns a promise which will resolve to the data or throw an exception.

This introduces a subtle quirk where you are actually handling the same exception in multiple different ways. Both in the hook/unwrap() function as well as the queryFulfilled callback.

For pessimistic updates, The docs (linked above) suggest wrapping the entire function in a try/catch. This is problematic because now any potential issues outside of the queryFulfilled are swallowed away and are very difficult to debug. If you decide to add logging in the catch block, you will be logging any fetch errors as well.

Because of this I removed the try / catch block. A few days later this resulted in a difficult to debug test failure where the fetch error that jest is reporting as 'isUnhandledError: false' is causing an unhandled error without a stack trace. Much time was spent looking at the unwrap code only to realise an embarrassingly long time later that the error is actually being thrown from the onQueryStarted callbacks.

My final resolution was to update every onQueryStarted as seen below: From:

let res = await queryFulfilled
// ... dispatch logic

To:

let res = await queryFulfilled.catch(e => ({error: e}))
if (error in res) return;
// ... dispatch logic

Obviously this adds a bit of boilerplate, but at least now I am only alerted to errors I should actually be investigating, and also not swallowing errors that I need to look into. {data} | {error} makes sense as a default as you shouldn't need to set up exception handling in multiple places for the same errors. If there is a reason for handling the same exception multiple times, I think the docs should be updated to outline some of the potential issues.

[phryneas]

Honestly, I'm not very sure what your problem is exactly. It seems to revolve around this statement:

This is problematic because now any potential issues outside of the queryFulfilled are swallowed away and are very difficult to debug.

But I honestly have no idea what you mean by that. Can you give an example?

PS: generally this reads as a critique of the concept of try..catch per se. Of course you can handle that differently as you have shown, but unlike other languages like go, try..catch is just the error handling mechanism that is most "native" in JavaScript and most devs are most familiar with that.

[joel1st]

Thanks for the reply @phryneas . You are correct in that it revolves around that statement. From the redux docs I linked, this is the example:

try {
    const { data: updatedPost } = await queryFulfilled
    // ~~~~ Any potential error below will be swallowed away by catch ~~~~
    const patchResult = dispatch(
      api.util.updateQueryData('getPost', id, (draft) => {
        Object.assign(draft, updatedPost)
      })
    )
} catch {}

Typescript reduces the potential for errors, but runtime errors are still a real possibility. For example, what happens if you are consuming a third party API that changes its payload in a way where updateQueryData now throws an exception. If I use the docs implementation, I have no console feedback AND monitoring services like Sentry won't pick up on this error as it is handled. It would be super hard to determine an error has even happened because it will just result in some stale api state.

I can see how this would sound like a critique of try catch. But really this is more to align await queryFulfilled with the rest of async thunk (which RTKQuery is built on top of). See async thunk doc excerpt below:

I feel like if an exception was desired from await queryFulfilled (I'm not sure in what case someone would knowingly want it),.unwrap would permit that to happen.

[phryneas]

Okay, first the reasoning why cAT thunks don't throw:
They would require you to always catch in your runtime code or you would get an "uncaught promise rejection" in your components if you choose not to do anything with the return value. And not doing anything with the return value is perfectly fine - after all this is Redux and you should probably handle your errors in a reducer, not your component.

Actually, looking back, it would not have been necessary doing it like that (the query lifecycle works around that), but it is there historically and we cannot get rid of it. (If I could, I would. .unwrap() makes it better at least.)

Now, the promises in the query lifecycle fulfill additional requirements: they do not only throw on error, but also for "data flow" purposes. In onCacheEntryAdded, cacheDataLoaded will throw if cacheEntryRemoved triggers before the data was actually loaded. It is there to interrupt code flow. For the purpose of "interrupt code that comes later in the block", a throw is just the best tool the language offers. And for symmetry reasons (and because exceptions are pretty much the tool of the language JavaScript to deal with errors), similar patterns are also used in onQueryStarted.

All that said, your actual problem seems to be "what if I execute additional code that might also cause an error". And tbh, the answer to that is: "use an additional try..catch and handle that error".

try {
    const { data: updatedPost } = await queryFulfilled
    try {
      const patchResult = dispatch(
        api.util.updateQueryData('getPost', id, (draft) => {
          Object.assign(draft, updatedPost)
        })
      )
    } catch (e) {
      // handle whatever error you expect here
    }
} catch {
}

[joel1st]

@phryneas Thanks for the thorough explanation, I can appreciate trying to maintain consistent behaviour.

I guess I have just associated nested try / catches as a potential code smell, especially swallowed errors. At the risk of sounding pedantic, even with the example you provided, there is a subtle (but unlikely) error that could be swallowed (eg if object destructuring is not transpiled it could error in some browsers), and again you would not be alerted by error monitoring. The reason for me harping on about this is that these kind of bugs are potentially very difficult to debug, especially if you can't reproduce it locally.

I guess it just feels weird that one of the base cases for this callback requires this pattern, but I'll let this go :) Anyway in either case I think we should update the docs to either use a variation of the nested try catches you have provided, or something similar to my original post, so that exceptions are not hidden away. If I put up a PR, which would you think is more likely to be approved?

[phryneas]

If you don't want to nest catch blocks, you could still go for

let updatedPost 
try {
   updatedPost = (await queryFulfilled).data
} catch { return }

In the end that's pretty much up to you.

As for a docs PR, I'd say adding a paragraph warning that the snippet above might swallow errors and advice to handle errors if any more complex logic is called (or showing the above approach as an alternative) would be okay, but I'd keep the examples themselves as they are. This should just not be a concern for most uses. Especially as we are showing "optimistic updates" here. Any optimistic update should be overwritten by data fetched from the server a moment later.

[joel1st]

Thanks @phryneas - The code snippets we have been discussing have been pessimistic updates (they occur after the successful queryFulfilled), which will potentially not be followed by a server sync if it is used for optimization reasons. Optimistic updates as presented in the docs, would not run into the issue I have raised as the custom logic is handled outside of the try/catch in a reasonable way.

Unless I'm missing something, it should be a concern for almost all users that are implementing pessimistic updates? If they follow the current docs, they will not be aware of any errors that are introduced - how could they be?

Maybe I'm missing the point entirely, either way I appreciate you discussing this with me.

[phryneas]

Sorry, my fault, you're right on the "pessimistic" thing.

Tbh., I think you are overestimating the amount of bugs that actually can happen here. There should generally not be a lot going on.

Nevertheless, I'm thinking a bit... it could be a possibility to just not try..catch at all here and if the error that is thrown by onQueryStarted is exactly the same error as was passed into it by throwing from queryFulfilled, it could just be silently swallowed by the function invoking onQueryStarted - so it would be handled on a library side. 🤔

Of course, that would make it "less visible" that an error can happen here. It's a bit off tradeoff. What do you think? Also, @markerikson @Shrugsy @msutkowski I'd be very interested in your take on this.

[joel1st]

@phryneas - oh I completely agree, the likelihood of bugs happening here should be very low. It's more of a peace of mind thing and not second guessing when there is an issue elsewhere whether it is actually caused by something hidden away.

You're suggestion would handle my issues with this perfectly - quite a cool idea! I don't see a case where you would want the fetch error be an unhandled error, so this makes sense to me.

[vicks0704]

Hi. I'm using rtk query to upload data to server. But when I call .onwrap() to get the response payload or error, I get the following type error: "Attempted to assign to readonly property" What could be the cause?

[phryneas]

@vicks0704 I am 100% sure that this has nothing to do with the original issue. Please don't piggy-back on someone else, but open your own issue. Please also provide code - your description is not enough to give any feedback.

[timothy-pham]

Okay, first the reasoning why cAT thunks don't throw: They would require you to always catch in your runtime code or you would get an "uncaught promise rejection" in your components if you choose not to do anything with the return value. And not doing anything with the return value is perfectly fine - after all this is Redux and you should probably handle your errors in a reducer, not your component.

Actually, looking back, it would not have been necessary doing it like that (the query lifecycle works around that), but it is there historically and we cannot get rid of it. (If I could, I would. .unwrap() makes it better at least.)

Now, the promises in the query lifecycle fulfill additional requirements: they do not only throw on error, but also for "data flow" purposes. In onCacheEntryAdded, cacheDataLoaded will throw if cacheEntryRemoved triggers before the data was actually loaded. It is there to interrupt code flow. For the purpose of "interrupt code that comes later in the block", a throw is just the best tool the language offers. And for symmetry reasons (and because exceptions are pretty much the tool of the language JavaScript to deal with errors), similar patterns are also used in onQueryStarted.

All that said, your actual problem seems to be "what if I execute additional code that might also cause an error". And tbh, the answer to that is: "use an additional try..catch and handle that error".

try {
    const { data: updatedPost } = await queryFulfilled
    try {
      const patchResult = dispatch(
        api.util.updateQueryData('getPost', id, (draft) => {
          Object.assign(draft, updatedPost)
        })
      )
    } catch (e) {
      // handle whatever error you expect here
    }
} catch {
}

is there a way to return this error to the query or mutation itself calling it? example:

getPost: build.query<Post, number>({
      query: (id) => `post/${id}`,
      async onQueryStarted(id, { dispatch, queryFulfilled }) {
        try {
          const { data } = await queryFulfilled
        } catch (err) {
          // return error to getPost query's error
        }
      },
    }),

[phryneas]

You can access it there, but you can't change the error anymore - that code will run after everything already happened.

[timothy-pham]

I'm trying using

getPost: build.query<Post, number>({
      query: (id) => `post/${id}`,
      async onQueryStarted(id, { dispatch, queryFulfilled }) {
        try {
          const { data } = await queryFulfilled
        } catch (err) {
          //1. throw err
          //2. rejectWithValue(err)
        }
      },
    }),

but there doesn't seem to be the right way to return this error to getPost's error. I want to get and handle that error in component because i maybe call some api in onQueryStarted (example: markpaid api after createOrder):

const {data: allOrdersData, error, isError, isLoading} = useGetAllOrdersQuery();

Is that right way and is it possible? Thank you so much!

[phryneas]

As I said, no, it's not possible. At that point in time, the query has already been handled completely and maybe even already started rendering your component. You could use a queryFn for that, but onQueryStarted exists in parallel to your query, but is not part of it.