Skip to content
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

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

Closed
1 task done
celebro opened this issue Feb 10, 2023 · 6 comments · Fixed by #6783
Closed
1 task done

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

celebro opened this issue Feb 10, 2023 · 6 comments · Fixed by #6783
Labels

Comments

@celebro
Copy link

celebro commented Feb 10, 2023

What version of Remix are you using?

1.12.0

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

  • Yes

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: celebro/remix-http-error-example@3e644df

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
Copy link

thomas-sc commented Feb 22, 2023

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".

@github-actions github-actions bot added the needs-response We need a response from the original author about this issue/PR label Apr 23, 2023
@remix-run remix-run deleted a comment from github-actions bot Apr 30, 2023
@machour machour removed the needs-response We need a response from the original author about this issue/PR label Apr 30, 2023
@brophdawg11
Copy link
Contributor

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
Copy link
Contributor

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

@brophdawg11 brophdawg11 added the awaiting release This issue has been fixed and will be released soon label Jul 11, 2023
@github-actions
Copy link
Contributor

🤖 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
Copy link
Contributor

🤖 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
Copy link
Contributor

🤖 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!

@brophdawg11 brophdawg11 removed the awaiting release This issue has been fixed and will be released soon label Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants