-
-
Notifications
You must be signed in to change notification settings - Fork 10.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Bug]: child loader redirect overrides parent loader error #11805
Comments
I'm curious why you would expect to remain on the same URL with an error boundary?
So why would we stay on
This sounds like a potential bug - I don't think the error should persist if we redirected, so we can look into that. |
Maybe my example was too minimal, the real world use case was more something like The parent loader is responsible for checking everything related to fetching the parent entity (permissions checks, handling non-200 responses for that entity in a certain way etc), maybe it might want to throw a particular error so the error boundary can show a "parent not found" or "you do not have access to this parent" message. The child entity loader is independent of that. Maybe it has a redirect so If I go to
I think this isn't consistent with how exceptions are handled generally. If a parent route throws exception X and the child route throws exception Y the parent's exception is always chosen over the child's. Whereas with redirects, the child gets an opportunity to override the parent. So the hierarchy is no longer one way, the child can override the parent's behaviour. I think the redirect being "where we want to end up" is not necessarily the case if the parent loader has some business logic effectively blocking off its subroutes to particular users, and "another chance of loading successfully" doesn't apply there either, as the request won't succeed in any subroute. The comment on #11357 (comment) also seems to imply redirects don't work this way in the non-data router. |
It sounds like what you really want is middleware so you can make these types of top-down decisions in a sequential manner. Loaders running in parallel introduces this type of "who wins" decision point - and the current design is to prioritize redirects over errors. |
I looked into this and the error is not sticking around after all. . You throw a 400 but you are seeing a net-new 404 error thrown by the router because no |
I am going to close this out since this is all functioning as designed and I believe the solution for your use case is middleware, which should hopefully be coming early in v7. |
That's strange, I was definitely hitting this but now I can't replicate it (I have a screenshot in #11357 (reply in thread), although the demo worked differently then). But confirming I am not seeing that behaviour now. |
OK, fair enough. Thanks for getting back to me and taking the time to look into it. |
What version of React Router are you using?
6.24.1
Steps to Reproduce
Set up a data router with a parent loader that throws an exception and a child loader that throws a redirect.
An example is here: https://stackblitz.com/edit/github-ypg2nu-lgqn1u?file=src%2Fapp.tsx
The
/
route has a loader withthrow new Response(null, { status: 400 });
.The
index: true
route underneath that has a loader withthrow redirect('/bar');
.Start the app as
/
as normal.Expected Behavior
The user sees the error boundary with the 400 request as the error and the current path as
/
, since the loader that threw the error is higher in the route hierarchy than the child. The redirect is ignored.Actual Behavior
The user sees a mix of behaviour - the error is set to the 400 request from the parent loader, but they've been redirected to
/bar
(the child loader).I had a look into what was going on. findRedirect finds the lowest redirect and then shortcircuits with that redirect, and seems to be run before any errors are handled. That would explain why the redirect is taking priority over the error.
I tried changing this to only return a redirect if there weren't any errors, and then also changed processRouteLoaderData to ignore any redirects instead of throwing.
However that caused the "reloads all routes when a loader during an actionReload redirects" submissions test to fail, and I had trouble figuring out why it was expecting what it was. The fact that the 400 error code is getting through even though the redirect shortcircuits everything seemed to indicate this was a bit trickier to fix than I thought.
See #11357 for more information.
The text was updated successfully, but these errors were encountered: