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

Set HttpStatusCode 500 in ErrorBoundary #1204

Closed
wants to merge 5 commits into from
Closed

Conversation

frenzzy
Copy link
Contributor

@frenzzy frenzzy commented Jan 1, 2024

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • infrastructure changes
  • Other... Please describe:

What is the current behavior?

In case of errors response status code is 200 OK

What is the new behavior?

In case of errors response status code is 500 Internal Server Error

Other information

'background-color': 'rgb(153, 27, 27)',
'border-radius': '5px',
padding: '4px 8px',
'margin-top': '8px',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With some space between error message and retry button looks prettier

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually changed all these styles to strings intentionally to reduce compiled size output.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed back to string.
I just thought styles are compiled to string during the build time...

@lxsmnsyc
Copy link
Member

lxsmnsyc commented Jan 1, 2024

I cannot agree to this because some errors can be intentional, and some unintentional. Not sure what others think.

@frenzzy
Copy link
Contributor Author

frenzzy commented Jan 1, 2024

Yes, but for intentional errors, you'll likely create a custom ErrorBoundary to suit your specific needs. For instance, you might not want to display the stack trace for these errors, and you'll want the error message to match the style of your site.

Alternatively, we could add support for customizing the status code. For example:

- <HttpStatusCode code={500} />
+ <HttpStatusCode code={props.error.statusCode ?? 500} />

However, I believe that defaulting to a 500 status code in error scenarios is generally better, as it's a widely accepted standard in web development.

@nksaraf
Copy link
Member

nksaraf commented Jan 3, 2024

I think it's reasonable the the default ErrorBoundary has this as the default. Its more web friendly and better overall for keeping up the semantics. I do agree that it should be trivial to override with your own ErrorBoundary (which people should be encouraged to know anyway)

@ryansolid
Copy link
Member

I think the intent here is now captured with the new dev overlay and default errorboundary. So while I won't be merging this. It is getting in.

@ryansolid ryansolid closed this Jan 4, 2024
@frenzzy
Copy link
Contributor Author

frenzzy commented Jan 4, 2024

Am I correct in understanding that by default, SolidStrat will not alter response status codes in the case of errors, since the primary audience for the default ErrorBoundary is developers, and it is designed to be most visible to them? In other words, are developers always expected to create their own ErrorBoundary to override the default behavior?

Would it be useful if the default ErrorBoundary displayed a message to suggest this practice?

Also, is there a possibility to tree shake away the default ErrorBoundary when a custom one is used?

@frenzzy frenzzy deleted the patch-2 branch January 5, 2024 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants