isLoading doesnt become true when state=pending, even if there was never any data due to an error on a previous fetch in 2.2.5

[Paul-Yves]

Just upgraded from 2.2.3 to 2.2.5

Previously we had a check doing if (isLoading || isError) return null;, it seems with a 401 error where we previously had isError to true, it is now false. Strangely, isFetching is now true (although error is present with a 401 in status).

I don't see a relevant change in the patch note, have I missed something or is it a bug ?

[markerikson]

Hmm. We did tweak that logic in #4364 - maybe related? (listed as "The isLoading flag logic was improved to handle errors when a query hook tries to subscribe." in https://github.com/reduxjs/redux-toolkit/releases/tag/v2.2.4 )

[david10sing]

Ok this is a major issue.

I've just had the similar issue where when an endpoint errors, it isError is set to true. However, when it is refetched, it is set to false.

Downgrade to v2.2.3 and the behaviour is correct.

We have most of our application doing validation using isError and I can foresee many parts of the application crashing instead of showing an error component.

[markerikson]

As usual, we would really need a repro or a Replay ( https://replay.io/record-bugs ) in order to be able to see what's going on.

Doubly so because I'm looking at the diff between 2.2.3 and 2.2.5 and barely seeing any actual logic changes in RTKQ:

https://github.com/reduxjs/redux-toolkit/compare/v2.2.3..v2.2.5

[david10sing]

@markerikson

Does this do the trick? Let me know if you need something else.

https://codesandbox.io/p/sandbox/cranky-platform-vk2myz?file=%2Fsrc%2Ffeatures%2Ftime%2FTimeList.tsx%3A102%2C4

Made changes to src/features/time/TimeList.tsx.

To reproduce

1. Block one of the time endpoint
   
2. Try to access that endpoint
3. Have a look at the console logs

[markerikson]

Gotcha, thanks.

I'll be up front and say I'm pretty tired atm and don't have the brainpower to look into this this evening. I might be able to look at it this weekend, but can't guarantee that.

[david10sing]

All G. I will pin to v2.2.3.

Thank you for the prompt reply.

[david10sing]

I had a look at this comment and the PR which introduced the changes.

Shouldn't the ParentComponent have an isError guard? If the parent component API is erroring, what would make the children request API request not error.

If I remove the change, isError works like how it used to.

[phryneas]

If I'm honest, I'm irritated how that would have any influence on isError though - none of that code modifies that value.

Could it be that the behaviour of isLoading changed here and as a result if (isLoading || isError) return null; went into the isLoading before and does not now - while isError doesn't change at all in your code? Could you please double-check those in isolation in the old version vs the new one?

[david10sing]

Hi @phryneas

In my code, we use isLoading and isError separately. I logged isError and I can confirm that it is false when there's actually an error coming from the endpoint.

Doesn't this codesandbox show the issue? https://codesandbox.io/p/sandbox/cranky-platform-vk2myz?file=%2Fsrc%2Ffeatures%2Ftime%2FTimeList.tsx%3A102%2C4

Our components are setup like this

const { isLoading, isError, data } = useGetSomeQuery()

if (isLoading) return <Loading />

if (isError) return <Error />

const {property} = data

return <Component {...property} />

If the query failed and errored, then the first time shows Error but re-rendering the component, the second time the query resolves, the isError is not false, data is undefined and we get an error.

When I revert this change https://github.com/reduxjs/redux-toolkit/compare/v2.2.3..v2.2.5#diff-73c1158d8e3a3d7e65694702b3a588f33308713c72ac94cf478f341dd4cf0494R693, everything works as expected.

[Paul-Yves]

I confirm on our side the issue is definitely with isError being false even if the request failed with a 401, it was set to true in the previous 2.2.3 version we used

[phryneas]

I'm sorry for the slow response time - I just moved and now I have to take care of a small puppy here, so while that's a wonderful experience, I barely have the time to do anything beyond that.

---

I am sorry, but I am still not 100% sure that we have cornered this bug, and I still believe we are looking at it from the wrong direction.

The reason for that is simple: no code that modifies isError in any way has been touched in these versions.

isError is calculated in a selector, and isLoading/isFetching are derived in a child selector in a hook. Only the latter code has been touched, not the former.

So while now there might be situations where an overlap between those might look differently, it's hard for me to believe that at any point in time isError would behave differently.

I believe that right now, a narrow error description hinders us from seeing the actual problem - and we need to understand the underlying problem to fix this. We can't just roll back the other change, as it itself fixed a bug.

With the demo, I have to be honest, I'm not sure what I'm looking at. Because there are three queries on the page, the successful queries seem to come from the other two queries, and after a (honestly, suspiciously long) waiting time, the impacted query flips to "error".

Could someone maybe take a stab at extracting a test that shows this behaviour?

[david10sing]

Thanks @phryneas

Could you point me towards the selector for isError please. I could take a look at this.

I shall try and make a smaller demo and will post soonish.

Thanks again.

[markerikson]

@david10sing : it's calculated here, separately from the hooks, and stored directly in the Redux state:

redux-toolkit/packages/toolkit/src/query/core/apiState.ts

Line 77 in a9362fb

isError: status === QueryStatus.rejected,

[david10sing]

https://codesandbox.io/p/sandbox/rtk-q-iserror-error-7ygljm?file=%2Fsrc%2Fservices%2Fpokemon.ts%3A10%2C45

Here's a simpler sandbox.

To reproduce:

1. Load the page
2. Block the bulbasaur endpoint
3. Click Some other tab
4. Click on Details
5. Notice this error
   

[phryneas]

Ah, so the scenario is

• we are in state=error, we were never in a success state, so we have no data
• we enter state=pending
• isLoading doesn't become true (due to fix: ensure hook subscription failures do not reset isLoading state #4364), but isError becomes false (as it did since the initial release of RTKQ).

[phryneas]

Do you want to do a PR?

The fix would probably be something like

-const isLoading = (!lastResult || lastResult.isLoading || lastResult.isUninitialized) && !hasData && isFetching
+const isLoading = (!lastResult || lastResult.isLoading || lastResult.isUninitialized || lastResult.error) && !hasData && isFetching

[david10sing]

I can do the PR. I had that lastResult.error as a solution but this (!lastResult || lastResult.isLoading || lastResult.isUninitialized || lastResult.error) looks very daunting.

Also why does the isError become false? On the first error, isError is true. But when we go back to the error component, it becomes false?

[phryneas]

Also why does the isError become false? On the first error, isError is true. But when we go back to the error component, it becomes false?

Because at that point in a time, we are status: 'pending', not status: 'error'.

Going back over #1349 and #4364, I'm starting to believe that change should never have been made - instead we should probably have tweaked the docs. The state we are in currently breaks all guarantees that we make in TypeScript - like if isLoading, isUninitialized and isError are false, data has to be set.

@smacpherson64 pulling you in here - I fear we'll have to undo that PR, as it was breaking a much more common use case. I'm sorry 😞

[smacpherson64]

Understood and no worries! Sorry to have caused this trouble! For now, for the current use case will likely do something like: #4364 (comment)

Does the initial concept of checking specifically for a previous error have the same issue?
packages/toolkit/src/query/react/buildHooks.ts

EDIT: adds link to previous discussion
EDIT: adds inquiry to see if checking for current or previous errors could work for both use cases.

[smacpherson64]

Understood and no worries! Sorry to have caused this trouble! For now, for the current use case will likely do something like: #4364 (comment)

Does the initial concept of checking specifically for a previous error have the same issue? packages/toolkit/src/query/react/buildHooks.ts

EDIT: adds link to previous discussion EDIT: adds inquiry to see if checking for current or previous errors could work for both use cases.

Thinking through this again, I do not believe it would. In both cases isLoading is always false after the first load. Since this is false, refetching after an error will still throw data.name because isError switches to false as well while pending.

If I am understanding this, the is states of the hooks are all current state (ignoring any past state).

isUninitialized: boolean // Query has not started yet.
isLoading: boolean // Query is currently loading for the first time. No data yet.
isFetching: boolean // Query is currently fetching, but might have data from an earlier request.
isSuccess: boolean // Query has data from a successful load.
isError: boolean // Query is currently in an "error" state.

Is the following breakdown correct?

when isUninitialized is true: 
  * isLoading = false
  * isFetching = false
  * isSuccess = false
  * isError = false
  
when isLoading is true:
  * isUninitialized = false
  * isFetching = true
  * isSuccess = false
  * isError = false
  
when isError is true:
  * isUninitialized = false
  * isLoading = false
  * isFetching = false
  * isSuccess = false

when isSuccess is true:
  * isUninitialized = false
  * isLoading = false
  * isFetching = false
  * isError = false

Are you thinking the documentation should become something like:

isLoading: boolean // Query is currently loading. No successful data has loaded yet.

Edit: thoughts on is state of documentation.