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

Unexpected behaviour of isSuccess on second refetch #4240

Open rusnick31 opened 7 months ago

rusnick31 commented 7 months ago

I have a simple endpoint getData https://stackblitz.com/edit/react-ts-7vwdac?file=App.tsx

const delay = () => {
  return new Promise((resolve) => {
    setTimeout(resolve, 500);
  });
};

let count = 0;

const api = createApi({
  reducerPath: 'api',
  endpoints: (builder) => ({
    getData: builder.query<number, void>({
      queryFn: async () => {
        count += 1;

        await delay();

        if (count > 1) {
          return { error: true };
        }

        return { data: 123 };
      },
    }),
  }),
  baseQuery: fakeBaseQuery(),
});

const { useGetDataQuery } = api;

which is used like that

function Component() {
  const { refetch, data, isError, isSuccess } = useGetDataQuery();

  console.log({ isError, isSuccess });

  return (
    <div>
      <button onClick={refetch}>refetch</button>
      <span>{data}</span>
    </div>
  );
}

export default function App() {
  return (
    <Provider store={store}>
      <Component />
    </Provider>
  );
}

output on mount is:

{ isError: false, isSuccess: false }
{ isError: false, isSuccess: false }
{ isError: false, isSuccess: true }

after calling refetch for the first time:

{ isError: false, isSuccess: true }
{ isError: true, isSuccess: false }

but if I call refetch one more time, I'll get

{ isError: false, isSuccess: true }
{ isError: true, isSuccess: false }

RTK switches isSuccess back to true for some reason while the request is loading

Is it the correct behaviour? Shouldn't isSuccess stay false?

EskiMojo14 commented 7 months ago

This appears to be intentional behaviour - useQuery holds onto the last successful request's data, and isSuccess is true if it has data and is currently fetching.

I don't fully understand the motivation behind this, though - hopefully somebody else can explain.

https://github.com/reduxjs/redux-toolkit/blob/ed9fc19027a31afee2234dea54fb36c8beef46ad/packages/toolkit/src/query/react/buildHooks.ts#L684-L695

markerikson commented 7 months ago

Until the in-flight request returns, we do have valid data from the last completed request, so the current status is still "successful".

rusnick31 commented 7 months ago

Thanks for your replies!

I just need to know how many errors have occurred to show the user different error messages

const Component = () => {
    const { isError, errorCount } = useQuery();

    if (isError) {
       return errorCount > N ? 'please contact support' : 'retry button';
    }
}

At first I tried to do it this way, but stumbled upon the issue with isSuccess

const useErrorCount = ({ isError, isSuccess }) => {
   const [errorNum, setErrorNum] = useState(0);

   useWhenValueChanges(isError, () => {
        setErrorNum(errorNum + 1);
   });

   useWhenValueChanges(isSuccess, () => {
        setErrorNum(0);
   });
}

so I rewrote it like that

const useErrorCount = (status: QueryStatus) => {
  const [errorsNum, setErrorsNum] = useState(0);

  useWhenValueChanges(status, () => {
    if (status === QueryStatus.rejected) {
      setErrorsNum(errorsNum + 1);
    }
    if (status === QueryStatus.fulfilled) {
      setErrorsNum(0);
    }
  });

  return errorsNum > N;
}

But now I can't just call useData() in different components and expect errorCount to be in sync

const useData = () => {
  const { status, isError } = useQuery();
  const errorCount = useErrorCount(status);

  return { isError, errorCount }
}

// Component1.tsx
const { errorCount } = useData();
// Component2.tsx
const { errorCount } = useData();

// render Component1, fail -> refetch -> fail, errorCount = 2
// then render Component2 (errorCount is brand new)

I can probably store errorCount in redux and use fulfilled and rejected matchers, but It looks like a lot of pain

const api = createApi({
  reducerPath: 'api',
  endpoints: (builder) => ({
    getData: builder.query<number, void>({
      queryFn: async () => { ... },
    }),
  }),
  baseQuery: fakeBaseQuery(),
});

export const { matchRejected, matchFulfilled } = api.endpoints.getData;

createSlice({
  extraReducers: (builder) => {
    builder.addMatcher(matchFulfilled, (state) => {});
    builder.addMatcher(matchRejected, (state) => {});
  },
});

Is there a better way to add errorCount?

markerikson commented 7 months ago

Storing the value in a Redux slice is the right approach. Tracking it at the component level won't give you a correct or consistent number of all possible errors that occurred.

(I'm admittedly also not sure how watching for 2 specific action types in a reducer is "a lot of pain" - that's very normal Redux logic.)

rusnick31 commented 7 months ago

I'm admittedly also not sure how watching for 2 specific action types in a reducer is "a lot of pain"

I think I would have to add this for every single request that I have ;) And maybe even some kind of subscription logic to reset the state when the last component that uses useData() hook gets unmounted

markerikson commented 7 months ago

At a minimum, you could genericize the matcher logic:

const isMyApiError = (action) => {
  return action.type.startsWith("myapi/") && isRejected(action);
}
phryneas commented 7 months ago

This appears to be intentional behaviour - useQuery holds onto the last successful request's data, and isSuccess is true if it has data and is currently fetching.

I don't fully understand the motivation behind this, though - hopefully somebody else can explain.

The data that's currently available in data is the data returned from a successful fetch. You probably don't want that to flicker away just because a new fetch starts.
At the same time, another fetch is going on -> both of these states are hit at the same time.

It's an attempt at simulating "suspense-like" behaviour, where you'd have the component in the last successfully rendered state, but at the same time had something like pending available to gray parts of the UI out or show a spinner.

That said, this "flickering" isSuccess doesn't seem intended to me.

rusnick31 commented 7 months ago

@markerikson the only thing that comes to mind is

const hasRTKQMeta = ( action: any ): action is { meta: { arg: { endpointName: string } } } => action?.meta?.arg?.endpointName;

export const referenceSlice = createSlice({
  extraReducers: (builder) => {
    builder.addMatcher(
      (action) => action.type.startsWith('apiCache') && isRejected(action),
      (state, action) => {
        if (hasRTKQMeta(action)) {
          state.errors[action.meta.arg.endpointName] += 1;
        }
      },
    );
  ...
  },
});
const useData = () => {
  const { endpointName, ...rest } = useQuery()
  const { errors } = useAppSelector((state) => state.slice);

  return { contactSupport: errors[endpointName] > N, ...rest };
}

+ some reset state logic on top of that hack ;)

I see that @tanstack/react-query has errorUpdateCount flag

  const { errorUpdateCount } = useQuery()

Is there any change something like this will end up in RTK as well?

markerikson commented 7 months ago

@rusnick31 not immediately likely. This is the first time anyone's ever asked for something like an error count that I know of. Individual feature requests are always a possibility, but we generally try to avoid adding things unless it feels like there's enough usefulness and a wide enough audience to justify adding something. (After all, any feature we ship adds bytes to bundle size for everyone.)

In the React-Query case, it looks like that error count was something they were already tracking internally for some reason and exposed it publicly. We don't keep track of error counts ourselves that I know of.

So, I'm not saying "no" permanently, but given that this seems doable in userland and this is the first time the question of "counting errors" has ever come up, I don't see an immediate reason to add this.