remix-run / remix

Build Better Websites. Create modern, resilient user experiences with web fundamentals.
https://remix.run
MIT License
29.73k stars 2.5k forks source link

Unexpected HTTP errors on _data calls cause blank routes without any caught errors #5418

Closed celebro closed 1 year ago

celebro commented 1 year ago

What version of Remix are you using?

1.12.0

Are all your remix dependencies & dev-dependencies using the same version?

Steps to Reproduce

Force a route's _data request to fail with an HTTP error and a response that remix doesn't expect (e.g. some string). This could happen when there is something wrong with e.g. CDN, proxy or load balancer configuration, an error that is generated outside or remix's control. Then make a client side navigation to that route. Here's a diff from base create-remix app with express server where I respond with an error from express handler: https://github.com/celebro/remix-http-error-example/commit/3e644df76e4e673a86cbdaecae4e138386ccb682

Expected Behavior

Remix should trigger ErrorBoundary with some generic error.

Actual Behavior

Remix renders an empty route, without any indication of an error.

thomas-sc commented 1 year ago

Same problem with Remix and Cloudflare CDN Messages (Rate Limit, WAF etc.). How are WAF errors or Captchas to handle that the client receives via the data request (_data) ? Depending on the status code (>=500?) you might have to redirect to the complete html page to see the original message or the captcha? Catch Boundary and Error Boundary seem to work only if the response header contains "x-remix-error" or "x-remix-catch". I can return a json in the Cloudflare WAF Rate Limit Response and also change the status code, but unfortunately I cannot add a header "x-remix-catch".

brophdawg11 commented 1 year ago

Hm, yeah this is tricky. The current behavior is as such because you can return anything from your loader, and as long as you don't throw that will make it through as loader data:

export function loader() {
  // This will make it to `useLoaderData`
  return new Response('whatever you want', { 
    status: 500,
    headers: {
      'Content-Type': 'text/plain'
    }
  });
}

Basically, we don't assume that 4xx/5xx should go to the ErrorBoundary - we let return/throw distinguish that.

So, if you get a 5xx response from somewhere other than the loader (i.e., CDN, Express, etc.), we can't distinguish that from a user-returned 500 at the moment.

My initial gut reaction is to add an X-Remix-Response header responses coming from loader/action calls on the server, and then if we don't see that client side we can automatically treat 4xx/5xx status code as errors since we know they they did not come from user-defined application loaders.

Somewhat related, I have seen folks ask for all 4xx/5xx to be automatically categorized as errors. I don't love this idea because it breaks from fetch behavior (and is closer to how axios handles it), but I can understand where the request is coming from. This would obviously be a big breaking change so would require a future flag if we went this route (I'm not confident this would win out).

brophdawg11 commented 1 year ago

This should be resolved by #6783 and available once 1.18.2 is released

github-actions[bot] commented 1 year ago

🤖 Hello there,

We just published version v0.0.0-nightly-c8936ac-20230712 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!

github-actions[bot] commented 1 year ago

🤖 Hello there,

We just published version 1.19.0-pre.0 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!

github-actions[bot] commented 1 year ago

🤖 Hello there,

We just published version 1.19.0 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!