reduxjs / redux-toolkit

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

Keep `isSuccess` consistent when refetching after an error #4732

Open markerikson opened 2 days ago

markerikson commented 2 days ago

This PR:

Fixes #4240

Investigation

Per #4240, if you have a sequence where:

the observed behavior is:

      [
        // Initial renders
        { id: 1, isFetching: true, isSuccess: false, isError: false },
        { id: 1, isFetching: true, isSuccess: false, isError: false },
        // Data is returned
        { id: 1, isFetching: false, isSuccess: true, isError: false },
        // Started first refetch
        { id: 1, isFetching: true, isSuccess: true, isError: false },
        // First refetch errored
        { id: 1, isFetching: false, isSuccess: false, isError: true },
        // Started second refetch <-------- `isSuccess` flips true here!
        { id: 1, isFetching: true, isSuccess: true, isError: false },
        // Second refetch errored
        { id: 1, isFetching: false, isSuccess: false, isError: true },
      ]

The issue is that isSuccess was false when the first refetch settled, but then flips to true when we start the second refetch. This does seem unintuitive and buggy.

Similar to #4731 , I think the logic for calculating isSuccess isn't sufficient. In this case:

Given that, I think the right thing to do is include && !lastResult?.isError, leaving us with:

    const isSuccess =
      currentState.isSuccess ||
      (hasData &&
        ((isFetching && !lastResult?.isError) || currentState.isUninitialized))

I see that flip the second refetch case to stay with isSuccess: false as I'd hope.

codesandbox[bot] commented 2 days ago

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders
Open Preview

netlify[bot] commented 2 days ago

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

Name Link
Latest commit 311f31c1927fdb4d8c8369e0a9933b693951f8cf
Latest deploy log https://app.netlify.com/sites/redux-starter-kit-docs/deploys/674277d99b5e7800089334db
Deploy Preview https://deploy-preview-4732--redux-starter-kit-docs.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codesandbox-ci[bot] commented 2 days ago

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 311f31c1927fdb4d8c8369e0a9933b693951f8cf:

Sandbox Source
@examples-query-react/basic Configuration
@examples-query-react/advanced Configuration
@examples-action-listener/counter Configuration
rtk-esm-cra Configuration
github-actions[bot] commented 2 days ago

size-limit report πŸ“¦

Path Size
1. entry point: @reduxjs/toolkit/query/react (cjs, production.min.cjs) 24.45 KB (-0.34% πŸ”½)
2. entry point: @reduxjs/toolkit/query/react (without dependencies) (cjs, production.min.cjs) 2.87 KB (+0.28% πŸ”Ί)
3. createApi (react) (.modern.mjs) 15.66 KB (+0.06% πŸ”Ί)