-
Notifications
You must be signed in to change notification settings - Fork 10.3k
[Blazor] Error page support for Server Side Rendered Razor Component Applications #50550
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
Conversation
src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerOptions.cs
Outdated
Show resolved
Hide resolved
Looks fantastic. I guess what remains is:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems potentially double edged. While Blazor requires a new scope, other components may be expecting the original scope, or the parent / request scope.
If you turn this on by default and run all the CI tests, does anything fail?
src/Components/Endpoints/src/Rendering/EndpointHtmlRenderer.Streaming.cs
Outdated
Show resolved
Hide resolved
src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerOptions.cs
Outdated
Show resolved
Hide resolved
src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerOptions.cs
Outdated
Show resolved
Hide resolved
cc20ff0
to
bd6134c
Compare
We can consider the error handling path "a separate request" the moment we send it down the pipeline. In that sense I don't think it'll be common for a component to fail in that situation, as it is "really hard" to distinguish that request from a regular request. What I mean by that, is that I expect most components to be completely ignorant of the new scope and behave in the same way as if it was the original request scope. To add to this, I turned on the create new scope by default, and I couldn't see any test fail outside of our existing flaky tests. In any case, as @SteveSandersonMS was quick to mention, this is something that users opt-in to, so it should not impact any existing app. |
bd6134c
to
4f0f3e3
Compare
@Tratcher any other feedback before this goes in? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To add to this, I turned on the create new scope by default, and I couldn't see any test fail outside of our existing flaky tests. In any case, as @SteveSandersonMS was quick to mention, this is something that users opt-in to, so it should not impact any existing app.
If we expect it to just work then we should turn it on by default and leave the option for opting out. Users hate having to discover settings to make things work.
src/Components/Endpoints/src/Rendering/EndpointHtmlRenderer.Streaming.cs
Outdated
Show resolved
Hide resolved
3caaa32
to
5e0ae68
Compare
Hi @javiercn. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge. To learn more about how to prepare a servicing PR click here. |
Hi @javiercn. This PR was just approved to be included in the upcoming servicing release. Somebody from the @dotnet/aspnet-build team will get it merged when the branches are open. Until then, please make sure all the CI checks pass and the PR is reviewed. |
@javiercn can you retarget this to the rc2 branch & confirm the test failures are unrelated? |
d508293
to
25c7fb4
Compare
This reverts commit bd6134c.
1865c88
to
f7c08bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
src/Components/Endpoints/src/Rendering/EndpointHtmlRenderer.Streaming.cs
Show resolved
Hide resolved
src/Components/test/testassets/Components.TestServer/RazorComponents/Pages/Error.razor
Outdated
Show resolved
Hide resolved
src/Components/test/testassets/Components.TestServer/RazorComponents/Pages/Error.razor
Outdated
Show resolved
Hide resolved
src/Components/test/testassets/Components.TestServer/RazorComponents/Pages/Error.razor
Outdated
Show resolved
Hide resolved
src/Components/test/testassets/Components.TestServer/RazorComponents/Pages/Error.razor
Outdated
Show resolved
Hide resolved
src/Components/test/testassets/Components.TestServer/RazorComponents/Pages/Error.razor
Outdated
Show resolved
Hide resolved
…onents/Pages/Error.razor
…onents/Pages/Error.razor
…onents/Pages/Error.razor
Description
Adds support for Error pages to Blazor Web application.
Fixes #49853, #49854
Customer Impact
which adds significant complexity to their setup.
Regression?
Risk
Verification
Packaging changes reviewed?