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

[Bug?]: Unable to set response status code inside ErrorBoundary #1201

Closed
2 tasks done
SuperGeekJake opened this issue Dec 31, 2023 · 4 comments
Closed
2 tasks done

[Bug?]: Unable to set response status code inside ErrorBoundary #1201

SuperGeekJake opened this issue Dec 31, 2023 · 4 comments
Labels
bug Something isn't working needs reproduction issue needs a reproduction

Comments

@SuperGeekJake
Copy link

SuperGeekJake commented Dec 31, 2023

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Current behavior 😯

Attempting to use the <HttpStatusCode code={err.statusCode ?? 500} /> component inside the fallback prop of an <ErrorBoundary /> still results in 200 status. The response is unaffected.

Expected behavior 🤔

The response status should be updated to match the H3Error.statusCode or default to 500.

Steps to reproduce 🕹

Not sure how to create a project on codesandbox to reproduce this, but this should be the relatvent code:

// @refresh reload
import { MetaProvider, Title } from "@solidjs/meta";
import { Router } from "@solidjs/router";
import { FileRoutes, HttpStatusCode } from "@solidjs/start";
import { ErrorBoundary, Suspense } from "solid-js";

export default function App() {
  return (
    <Router
      root={(props) => (
        <MetaProvider>
          <Title>Example</Title>
          <Suspense>
            <ErrorBoundary
              fallback={(err, reset) => (
                <>
                  <HttpStatusCode code={err.statusCode ?? 500} />
                  <main>
                    <h1>Error</h1>
                    <p>{err.message}</p>
                    <button onclick={reset}>Reset</button>
                  </main>
                </>
              )}
            >
              <Suspense>{props.children}</Suspense>
            </ErrorBoundary>
          </Suspense>
        </MetaProvider>
      )}
    >
      <FileRoutes />
    </Router>
  );
}

Context 🔦

Attempting to handle any page errors by wrapping the file router in an ErrorBoundary. Besides unhandled server errors, I'm also intentionally throwing 404 H3Errors from the RPC server functions when the requested URL params don't point to valid data. Currently, its allowing me to show an error page, but the response status is still 200.

Your environment 🌎

System:
  OS: Windows 11 (WSL Ubuntu 20.04)
  Node: v20.6.1
  Browser: Chrome 120.0.6099.130
Packages:
  @solidjs/meta: ^0.29.3
  @solidjs/router: ^0.10.5
  @solidjs/start: ^0.4.2
  solid-js: ^1.8.7
  vinxi: ^0.0.54
@SuperGeekJake SuperGeekJake added the bug Something isn't working label Dec 31, 2023
@frenzzy
Copy link
Contributor

frenzzy commented Jan 1, 2024

I just added <HttpStatusCode code={500} /> to the default ErrorBoundary component (see PR #1204). After this change, I noticed that it started returning the correct status code, as you can see in this screenshot:
Screenshot 2024-01-01 at 13 46 22

However, I observed that the onBeforeResponse middleware executes before the ErrorBoundary component. This behavior might be related to the issue, and it's possible that an await is missing somewhere, which could explain why it sometimes works correctly.

@ryansolid
Copy link
Member

If the response is streaming then its possible I assume that onBeforeResponse could fire before the ErrorBoundary and the status code would never update. But it shouldn't happen in the synchronous case.

@ryansolid
Copy link
Member

Yeah more specifically. Given the new ErrorBoundary works and I haven't reproduced it locally my suspicion is that this is a streaming response that has already flushed. Please provide a reproduction. Stackblitz should work now or a github repo.

@ryansolid ryansolid added the needs reproduction issue needs a reproduction label Jan 5, 2024
@SuperGeekJake
Copy link
Author

I wasn't able to replicate when using the latest version of solid-start and vinxi, works as expected 🙌 !
https://stackblitz.com/edit/github-huk5bu?file=src%2Fapp.tsx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs reproduction issue needs a reproduction
Projects
None yet
Development

No branches or pull requests

3 participants