reduxjs / redux-toolkit

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

RTK Query gets stuck pending and swallows errors thrown in addMatcher reducers #3795

Open adamerose opened 11 months ago

adamerose commented 11 months ago

I just ran into this problem and found this comment saying it's expected behavior, but I'm making this issue with a clearer example for further discussion because this seems like a footgun that should be changed.

Below is an example with a login button. Click the button and it sends a POST request to the backend with credentials to log in and gets back user info + token, and stores them in the authSlice and displays the username.

https://codesandbox.io/p/github/adamerose/rtkq-reducer-error-example/master https://github.com/adamerose/rtkq-reducer-error-example

With the error thrown in the matchFulfilled reducer, the query gets stuck forever in this state even though the HTTP request completed successfully:

isUninitialized:false
isFetching:true
isSuccess:false
isError:false
const authSlice = createSlice({
  name: "auth",
  initialState: initialState,
  reducers: {
    logout: (state) => {
      state.user = null
    },
  },
  extraReducers: (builder) => {
    builder.addMatcher(
      api.endpoints.login.matchFulfilled,
      (state, { payload }) => {
        state.user = payload.user

        throw Error("Example error")
      },
    )
  },
})

It also seems to completely swallow the error and nothing appears in the console, which made it very hard to figure out why I was getting stuck pending in an actual project. I've included a separate Throw Error button that throws an error in a normal reducer and I can see it appear in the console, but for some reason the one inside the addMatcher example doesn't. Am I doing something wrong here?

image image image
markerikson commented 11 months ago

At first glance, that sounds like normal Redux behavior.

If you look at the Redux core inside of dispatch(), it does:

    try {
      isDispatching = true
      currentState = currentReducer(currentState, action)
    } finally {
      isDispatching = false
    }

So any error in a reducer gets swallowed.

Generally speaking, reducers are supposed to be pure functions, and philosophically that means they shouldn't throw errors at all.

I get the point that you're making, and agree it would be conceptually nice to have a better indicator here. But there's only one root reducer function, which is called to return a new state, and if any code inside that reducer errors there isn't a state update applied at all.

How and why are you running into cases where a reducer is throwing an error?

adamerose commented 11 months ago

Generally speaking, reducers are supposed to be pure functions, and philosophically that means they shouldn't throw errors at all.

I'm only explicitly throwing an error in the code here as a minimal reproducible example. It's not like we can simply decide to never have any errors occur in a reducer right? Errors are unavoidable and not all errors are caused by side effects, maybe I just have a typo or try to access an undefined property.

How and why are you running into cases where a reducer is throwing an error?

Here is the code that initially caused this for me:

  extraReducers: (builder) => {
    builder.addMatcher(api.endpoints.login.matchFulfilled, (state, { payload }) => {
      state.token = payload.token;
      const decodedToken: AuthJwtToken = jwtDecode(payload.token);
      console.log(decodedToken, payload.token);
      state.username = decodedToken.username;
    });
  },

Not sure what the error was because it got swallowed...

So any error in a reducer gets swallowed.

Doesn't my Throw Error button demonstrate that isn't true for regular reducers?

image

Anyways, swallowing errors anywhere seems like a problem.

markerikson commented 11 months ago

Can you record a Replay ( https://replay.io/record-bugs ) of the original issue, and share that here? That would make it a lot easier for me to see what's going on.

adamerose commented 11 months ago

Unfortunately it's a company project so I can't share a replay - but I think my codesandbox or repo demonstrate the issue. Any errors inside a reducer in builder.addMatcher for matchFulfilled of an RTKQ query get swallowed and end up in with RTKQ having incorrect pending state.

You could replace throw Error("Example error in addMatcher") here with a typo like state.user = paylaod.user or a function call to an external library like lodash or anything else that might throw an error. Surely I can't be the first person trying to debug an error inside a reducer?

EskiMojo14 commented 11 months ago

Generally quite confused by this discussion.

At first glance, that sounds like normal Redux behavior.

If you look at the Redux core inside of dispatch(), it does:

try {
  isDispatching = true
  currentState = currentReducer(currentState, action)
} finally {
  isDispatching = false
}

So any error in a reducer gets swallowed.

I don't see anywhere an error would be swallowed here? Errors thrown in a reducer will be uncaught as usual, as shown by the "Throw error" in the linked CSB. Though as covered, reducers should never throw unless something is very wrong.

Errors thrown in an addMatcher reducer shouldn't be caught either: Error reducer playground

Whatever's happening with regards to the error being swallowed seems very specific to RTKQ - either during its middleware, thunks, or hooks.

markerikson commented 11 months ago

If it helps, I just got a replay recorded:

https://app.replay.io/recording/rtk-3795-addmatcher-swallowing-an-an-error-in-a-reducer--dd7a6a80-fe98-49f9-a1a4-a39f3d5fe4bd

markerikson commented 11 months ago

Weird. It looks like the error goes all the way back up to the stack... and then gets swallowed by an async-to-generator-transpiled body in redux-toolkit.esm.js?

markerikson commented 11 months ago

And if I re-record the example with 2.0-beta.2 (which uses real async), it seems like we just unwind the stack out to createAsyncThunk and it vanishes:

https://app.replay.io/recording/rtk-3795-error-in-a-reducer-rtk-20-beta2--35fad959-c1d4-4ce2-8365-dabf3340ee6e

I'm actually pretty confused, tbh. It seems like the error does get thrown upwards, and then just vanishes.

Unfortunately I'm busy enough that I don't have time to look into this further. If you or anyone else does and can figure out what's going on, I'd be curious to know details.

adamerose commented 11 months ago

I couldn't find it in your replay but I stepped through in my debugger locally and I'm pretty sure the error is getting swallowed here: https://github.com/reduxjs/redux-toolkit/blob/e657098ad8e8d13e3757b5c2cfe17c594320f6b9/packages/toolkit/src/query/core/buildInitiate.ts#L456

I commented that line out in node_modules and began seeing the error in my console. However this still doesn't fix RTKQ getting stuck pending.

image
markerikson commented 11 months ago

Ah, interesting. I assume that's to avoid repetitive console logs about uncaught errors.

Like I said, nothing can fix the "stuck pending" problem. The reducer was supposed to update the state, but there was an error in the reducer, so no state updates got saved for that action. That's architecturally defined - state = reducer(state, action) failed.

The answer to that truly is "don't have errors in a reducer".

adamerose commented 11 months ago

The reducer was supposed to update the state, but there was an error in the reducer, so no state updates got saved for that action. That's architecturally defined - state = reducer(state, action) failed.

Can you expand on this?

  1. What should my Redux state be after a reducer throws an error? Do we expect it to stay as it was before the reducer ran, or is it undefined (ie. depending on the error, our state could be anything and the app has to be hard restarted)?
  2. Should the action that triggered the failing reducer still show up in dev tools?
  3. Should the other reducers triggered by that same action all automatically fail too? It seems like this is the cause of RTKQ getting stuck pending here.
  4. Should there be some indication in the dev tools that a reducer/action failed with an error?

The answer to that truly is "don't have errors in a reducer".

That is always the goal, but if a reducer error does happen in production having things fail non-catastrophically would be ideal.

In the case of my original demo - updating the RTKQ query statusFlags isn't logically tied to my custom reducer for storing the auth response, they are just two separate things that need to happen when my HTTP request succeeds, so I would want my reducer failing to mean auth.user fails to update but not to also break RTKQ and have it get stuck as pending. What do you think of this as a potential solution?

Or an alternative solution would be to make it so that if one reducer throws an error, the other reducers for the same action still complete. I'm not sure if this goes against fundamental Redux architecture.

markerikson commented 11 months ago

What should my Redux state be after a reducer throws an error? Do we expect it to stay as it was before the reducer ran

Yes, exactly that. The reducer function threw, no new state was returned, therefore the store's currentState variable was not updated.

Should the action that triggered the failing reducer still show up in dev tools

Not 100% sure. The devtools piece is an enhancer that wraps around the original store.dispatch(). I assume that it only sends the action to the extension after the real store.dispatch() is complete, so that it can get the current state and serialize it. So, there also, if an error is thrown in the reducer, that enhancer code to get the state never runs.

Should the other reducers triggered by that same action all automatically fail too?

Yes, because there is one root reducer function, and all of the slice reducers are actually part of that one root reducer function.

Should there be some indication in the dev tools that a reducer/action failed with an error?

The devtools UI has no provision for that that I know of, and that ties into point 2.

updating the RTKQ query statusFlags isn't logically tied to my custom reducer for storing the auth response, they are just two separate things that need to happen when my HTTP request succeeds

But that's part of the point here.

The fulfilled action only got dispatched once. RTKQ depends on that.

You've added extra reducer code, that runs in the same root reducer, handling the same action. So when the one call to rootReducer(state, rtkqFulfilledAction) errors, that prevents the RTKQ fulfillment update from being saved too.

I get that this is a pain point. But it ultimately comes down to "when someFunction() throws an error, it doesn't return anything" as a basic piece of JS execution behavior, and in this case the function is actually rootReducer().

We aren't going to rearchitect RTKQ's internals just to try to work around the chance that a user might add extra reducers listening to the same action, and those reducers might someday throw an error. We have to write the code assuming the RTKQ reducer logic is the only code that cares about the action, and that our logic handles it correctly.

adamerose commented 11 months ago

You've added extra reducer code, that runs in the same root reducer, handling the same action. So when the one call to rootReducer(state, rtkqFulfilledAction) errors, that prevents the RTKQ fulfillment update from being saved too.

Okay that makes sense, so it sounds like the only fix would be separate actions like I described. Or in my own code I can wrap all reducers that touch internal RTK actions with a try/catch that returns initial state on failure.

We aren't going to rearchitect RTKQ's internals just to try to work around the chance that a user might add extra reducers listening to the same action, and those reducers might someday throw an error.

That's fair enough. Maybe reconsider it if you come across other problems caused by mixing internal and user actions, I agree this issue alone wouldn't justify the effort. If the error swallowing gets fixed then this becomes a much smaller pain point.

Thanks for all your work, and quick response time!

markerikson commented 11 months ago

Btw, not sure I actually got this detail: what was the actual error that was happening in your real code to cause this originally?

adamerose commented 11 months ago

In the code I pasted earlier:

  extraReducers: (builder) => {
    builder.addMatcher(api.endpoints.login.matchFulfilled, (state, { payload }) => {
      state.token = payload.token;
      const decodedToken: AuthJwtToken = jwtDecode(payload.token);
      state.username = decodedToken.username;
    });
  },

I had a mismatch in the types defined for my backend and frontend. My backend responded with an object with the access_token field but my frontend expected it in the token field. So despite having types in my api endpoint generics and TypeScript having no complaints, payload.token was undefined and calling jwtDecode(undefined) threw the error Invalid token specified.

adamerose commented 9 months ago

What's the reason this was closed?

To recap the thread...

phryneas commented 9 months ago

We're a bit at the mercy of how JavaScript handles errors here - every kind of error in an async context will throw in the same way through the same problem - we can't really distinguish between errors from different sources, and generally, errors should just end up in the store, but not bubble globally, or people would get "uncaught exception" warnings for expected errors.

The problem here is that this occurs in the exact spot where that error would also be written, so I don't really know what to do about it. I'll reopen this (it was one of many many issues that were closed as 2.0 was released), but I don't know a good solution.

markerikson commented 9 months ago

I was doing a cleanup sweep through our issues list and trying to close anything that looked resolved or unactionable.

Yeah, as we've discussed, there's nothing that can be done on the reducer side - that's inherent to how Redux works.

I'm not clear if there's anything we can do internally beyond that.

adamerose commented 9 months ago

errors should just end up in the store, but not bubble globally, or people would get "uncaught exception" warnings for expected errors.

Doesn't the codesandbox demo in my original question show that RTK normally does bubble errors up to the console where the user can see them? My issue is just about errors being swallowed in reducers specifically attached to internal RTK actions. I'm confused how this is a problem inherent to how Redux works, or why it would be unsolvable.

image

Can anyone comment on the snippet I mentioned here? https://github.com/reduxjs/redux-toolkit/issues/3795#issuecomment-1758806792 This seems to just silently swallow the error, or is that not what's going on here?

AndreaDigiTrails commented 5 months ago

This seems somewhat related to the issue that I have with a Next.js 14 application where I used RTK Query to handle the authentication phase of the application.

I still have to figure out where the main issue is but the thing is that I built a Context that wraps my layout.tsx. Inside it I use RTK Query to ping an endpoint (user/me) just to check if the user is authenticated.

On many of the routes this works just fine, if my BE respond with a 401, thanks to the re-authorization tutorial on RTK Query website I am able to perform a refresh API call but there is a specific URL/button that sometime just fails to work.

The URL I am trying to see is the default page that user sees after a successful login, see it as a 'Back home button', and it happen that when I try to see that route by clicking on a Link component it does not call the user/me endpoint but looks like Next.js tryes to load a cached version of the page.

My issue is that I do not see any error or enpoint pinged and it really confuse me.

Since I am not able to figure out what the issue is I am in the middle of a refactor aimed at removing the Redux/RTK Query dependency and handle it with a combination of server and client components with Next.js

I just wrote this comment in the hope that someone have faced something similar and is able to help me out.

If you need more details on how the application works I am adding them below but unfortunately I am not able to build a full example.

Little ReAuth difference

My fetchBaseQuery uses the credentials: 'include' as my BE is able to generate httpOnly cookies.

The Auth Context

This is the 'Context' that I use in order to handle the loading state of the application, and it is the one where I notice that's stuck on 'Loading...' when I try to check the URL mentioned above.

"use client";

import { useEffect } from "react";
import { redirect } from "next/navigation";
import { useAppSelector, useAppDispatch } from "@/redux/hooks";
import { setAuth, finishInitialLoad } from "@/redux/features/authSlice";
import { useUserQuery } from "@/redux/features/authApiSlice";

export default function RequireAuth({
  children,
}: {
  children: React.ReactNode;
}) {
  const { isAuthenticated, isLoading } = useAppSelector((state) => state.auth);
  const dispatch = useAppDispatch();
  const { status } = useUserQuery();

  useEffect(() => {
    if (status === "rejected") {
      redirect("/");
    }

    if (status === "fulfilled") {
      dispatch(setAuth());
      dispatch(finishInitialLoad());
    }
  }, [status, dispatch]);

  if (isLoading) {
    return <div className="flex justify-center my-8">Loading...</div>;
  }

  if (!isAuthenticated) {
    redirect("/");
  }

  return <>{children}</>;
}
markerikson commented 5 months ago

@AndreaDigiTrails based on your description I'm not sure you're seeing the same problem described in this issue.

That snippet isn't enough to actually give me any sense of what might be happening. Any chance you can share a CodeSandbox, Github repo, or Replay ( https://replay.io/record-bugs ) of the problem?