- 
                Notifications
    You must be signed in to change notification settings 
- Fork 10.5k
          Add NotFoundPage to Router
          #60970
        
          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
  
    Add NotFoundPage to Router
  
  #60970
              
            Conversation
        
          
                src/Components/Endpoints/src/Rendering/EndpointHtmlRenderer.EventDispatch.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                src/Components/test/testassets/Components.WasmMinimal/Routes.razor
              
                Outdated
          
            Show resolved
            Hide resolved
        
      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.
Looks great!
I think we will also want to update the project templates to use this feature.
In addition to it, I think there's another scenario we want to enable here which is to make sure that it works with UseStatusCodeWithReexecute
- UseStatusCodeWithReexecuteallows the app to render a page after a 404 has been set somewhere. We want that to also be possible with a Component page
I did something similar for handling errors a couple releases back. This is the PR with the change for reference #50550
- I suspect we will need to do something similar here.
/cc: @danroth27
        
          
                ...ectTemplates/content/BlazorWeb-CSharp/BlazorWeb-CSharp/Components/Pages/WeatherDetails.razor
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...b.ProjectTemplates/content/BlazorWeb-CSharp/BlazorWeb-CSharp/Components/Pages/NotFound.razor
          
            Show resolved
            Hide resolved
        
              
          
                src/Components/Endpoints/src/Rendering/EndpointHtmlRenderer.EventDispatch.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | { | ||
| context.Response.ContentType = RazorComponentResultExecutor.DefaultContentType; | ||
| var isErrorHandler = context.Features.Get<IExceptionHandlerFeature>() is not null; | ||
| var hasStatusCodePage = context.Features.Get<IStatusCodePagesFeature>() is not null; | 
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.
I think the interface we are looking for is IStatusCodeReExecuteFeature not IStatusCodePagesFeature. The latter is added all the time to the middleware, while the first one is only added by the handler when it's dealing with the re-execution code path, which I think is what we want (and is the equivalent of isErrorHandler)
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 complicates the logic a bit. We need a way to avoid writing the response (so return from RenderComponentCore method before flushing) if we expect re-execution. By "expect" re-executon I mean: status code page was set, we used to have a 200 but now we have 404. At the place where we would like to detect it, IStatusCodeReExecuteFeature is unset and we have no way of preventing starting the response.
We could set IStatusCodeReExecuteFeature for in the handler subscribed to OnNotFound event: in SetNotFoundResponseAsync. This forces us to add <Reference Include="Microsoft.AspNetCore.Diagnostics" /> to Microsoft.AspNetCore.Components.Endpoints.csproj. Is this acceptable?
Otherwise, we will return too early to handle re-execution, again this condition:
aspnetcore/src/Middleware/Diagnostics/src/StatusCodePage/StatusCodePagesMiddleware.cs
Line 67 in 55a1ae7
| if (context.Response.HasStarted | 
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.
Relying on IStatusCodeReExecuteFeature fails - the result is that we always flush the response and we never have a chance to re-execute.
The problem here is that we would like to detect the page that is going to re-execute, to prevent writing to its contents, not a page that already was re-executed.
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.
In error handler we don't have such a problem (we don't have to detect to return early) because the thrown exception does it for us. We never execute any code after RenderEndpointComponent in EH case. For 404 we have to return early based on a condition.
The most reasonable is probably IStatusCodePagesFeature->set & IStatusCodeReExecuteFeature->unset & responseCode->404.
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.
I don't fully follow, but I don't think we need to block on this
        
          
                ...ts/Components.TestServer/RazorComponents/Pages/StreamingRendering/StreamingSetNotFound.razor
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | if (_notFound == null) | ||
| { | ||
| // global router doesn't exist, no events were registered | ||
| throw new InvalidOperationException("No handler is subscribed to the OnNotFound event. Ensure that the application has a global router or manually subscribe to the OnNotFound event."); | ||
| } | 
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.
nit: I don't think this is blocking, but we shouldn't throw for this case, noop-ing is fine.
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.
Looks great, I think there are some things we need to tweak, but nothing critical we need to block on for preview4.

Description
NotFoundPageparameter that can be used to customize the view of page rendered onNotFoundEvent._notFoundwas done, so we use redirection to a hardcodednot-foundpage.NotFoundpage.app.UseStatusCodePagesWithReExecute("/reexecution-path", createScopeForErrors: true);Fixes #58815