[Bug]: throwing a response with headers should preserve the headers

[kentcdodds]

What version of React Router are you using?

6.4+

Steps to Reproduce

I put an example with Remix together for this, but I think the fix will be in the router which is why I'm opening this issue here (maybe the repos should be merged afterall?).

Here's the repo: https://github.com/kentcdodds/throw-response-headers

Here's a loom explaining the issue: https://www.loom.com/share/20a07a160bd54ec6bf228dcdc46f1ef3

Expected Behavior

The short of it is, if you stick this in a loader:

export async function loader() {
  throw new Response("", { status: 418, headers: { "x-my-header": "sup" } });
}

The x-my-header doesn't show up in the response.

But if you do this:

import { redirect } from "@remix-run/node";

export async function loader() {
  throw redirect("/", { headers: { "x-my-header": "sup" } });
}

The x-my-header does show up.

Actual Behavior

I would expect throwing a response to preserve the headers of that response (like it does with a redirect).

[brophdawg11]

Confirmed this isn't related to the RR integration - we still match 1.7.6 behavior (1.8.0 was the first release that started using the new router for server-side data fetching). It seems for document requests we always only handled headers from the "renderable matches", which is everything at the handling boundary and above. Here's the 1.7.6 logic and the associated 1.12.0 logic. If you add a CatchBoundary in response.tsx you can see the header making it back.

As for whether this is the right behavior I'll need to check with @mjackson and @ryanflorence. It makes sense if you think about it from the standpoint of "the leaf route headers function wins" since when the child doesn't have a boundary it's not even rendered. It could be confusing to render a parent CatchBoundary along with the throwing child's header() function value.

I'm going to transfer this over to Remix since it does turn out to be isolated there since the static handler is returning all the headers and we're just ignoring those below the boundary.

[brophdawg11]

FWIW the reason it works with redirect is because we just send the redirect Response back verbatim, and we don't have to deal with merging headers from multiple Response instances

[kentcdodds]

Thank you for letting me know of that workaround!

I tried a few variations and determined that if you want the headers to come through in this case you need:

1. A CatchBoundary at the route that throws the response (or, if you're using v2_errorBoundary, then it's an ErrorBoundary you need)
2. A headers export that forwards the custom headers along for the document headers

I think the current behavior makes sense, but I'm not certain. It kinda feels like we're getting half of a feature. Part of the throw new Response feature is that those responses bubble up to the nearest parent with a Boundary. But the headers don't bubble like the response does. So the UI bubbles, but headers don't. Even if the route that throws is a child and it has a headers export, that's ignored if the child doesn't have a Boundary export.

So we get response UI error bubbling, but not response header bubbling. I think I would be satisfied with the current behavior if the headers export also got a childLoaderHeaders option that it could use to combine headers (along with the loaderHeaders, actionHeaders, and parentHeaders). But that kinda feels wrong.

Instead, I think it would be better if the headers function that's called is always the one for the route that's matched, even if that route's loader fails. But that also feels kinda wrong because maybe we're not throwing a Response, maybe it's a runtime type error or something. So maybe the loaderHeaders could be an empty Headers in that case? (I think that's what it is if the page is rendered by an action instead).

All that said, I really don't think it should be necessary to put a headers export in every one of your routes. Especially for use cases like this where you could have an abstraction like requireUserWithPermissions which throws a 403 response and you want one parent to handle the UI and headers for that. Otherwise, any route that uses requireUserWithPermissions has to not only call it in their loader, but also must handle the response headers which could be easy to do 😬

Now, that use case may be handled by the upcoming context feature. Is it?

Anyway, I'm not certain what to recommend here, but the status quo feels wrong to me.

[brophdawg11]

All that said, I really don't think it should be necessary to put a headers export in every one of your routes

I agree here. I'd love to re-think the approach there and see if we could be smarter based on which matched routes actually exported a headers method and/or returned/threw a Response with headers. In the simplest case if only one loader specified headers, and no routes exported function headers(), then you could just use them and be done with it. Not sure if those heuristics hold up in more complex setups though...

Now, that use case may be handled by the upcoming context feature. Is it?

I think so? If you have a middleware on the parent with headers+boundary, then requireUserWithPermissions throwing there would technically throw from each loader (including parent) and thus the parent headers would get access to it form it's own thrown response, since it is the boundary route?

middleware will definitely change the discussion here a bit so it's worth letting that land before making any decisions on this specific behavior 👍

[github-actions]

This issue has been automatically marked stale because we haven't received a response from the original author in a while 🙈. This automation helps keep the issue tracker clean from issues that are not actionable. Please reach out if you have more information for us or you think this issue shouldn't be closed! 🙂 If you don't do so within 7 days, this issue will be automatically closed.

[kentcdodds]

This is definitely still relevant.

[brophdawg11]

Fixed by #6425 and should be available in 1.17.0. You'll have errorHeaders available in your leaf headers function.

Also worth noting we are going to get #5473 merged as well. That was never the intended behavior (and pre v1 it didn't work that way) but since it got changed (presumably unintentionally) pre-v1 we're going to stick it behind a future flag. So then we'll use the deepest renderable headers function if the leaf doesn't provide one.

[github-actions]

🤖 Hello there,

We just published version v0.0.0-nightly-3e093e5-20230520 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!