dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.37k stars 9.99k forks source link

NavigationManager.Refresh throws on SSR-only pages #51222

Closed SteveSandersonMS closed 10 months ago

SteveSandersonMS commented 1 year ago

It throws:

NotImplementedException: The type Microsoft.AspNetCore.Components.Endpoints.HttpNavigationManager does not support supplying NavigationOptions. To add support, that type should override NavigateToCore(string uri, NavigationOptions options).

As far as I know, NavigationManager.Refresh should be supported and should be equivalent similar (update: see below) to calling Nav.NavigateTo(Nav.Uri), which does work.

Repro 1

@page "/"
@inject NavigationManager Nav

<form @onsubmit="@(() => Nav.Refresh())" @formname="myform" method="post">
    <AntiforgeryToken />
    <button>Submit</button>
</form>

Repro 2

This is a repro for the "usability issue in the same area" mentioned below. If the form is updated to have data-enhance, then ideally the post-redirect-get flow should not append any extra browser history entries. That is:

Actual behavior: When the first issue is fixed, clicking "back" after submitting this form (with data-enhance added) will leave you on Home.razor because of the duplicate history entry Expected: It should take you to wherever you were before you arrived at the Home.razor page

SteveSandersonMS commented 1 year ago

A usability issue in the same area:

If you do NavigationManager.NavigateTo(Nav.Uri) while enhanced nav is enabled, it unconditionally pushes a new history entry, even if it's the same as where you already are. That may be OK for NavigateTo but it means it's not really suitable for implementing post/redirect/get.

There needs to be some way to do post/redirect/get without pushing a new history entry, otherwise "back" then doesn't work as you'd expect. We should check that NavigationManager.Refresh can be fixed to force the enhanced load but without pushing a new history entry.

ghost commented 1 year ago

Thanks for contacting us.

We're moving this issue to the .NET 9 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

surayya-MS commented 10 months ago

I was able to repro the bug.

Interestingly, if you choose Blazor Web App with Interactivity Auto instead of None, and try same thing then the error message is different. The error doesn't show on the page but in terminal:

Microsoft.AspNetCore.Components.Server.Circuits.RemoteNavigationManager[5] Failed to refresh System.ArgumentNullException: Value cannot be null. (Parameter 'jsRuntime') at System.ArgumentNullException.Throw(String paramName) at System.ArgumentNullException.ThrowIfNull(Object argument, String paramName) at Microsoft.JSInterop.JSRuntimeExtensions.InvokeVoidAsync(IJSRuntime jsRuntime, String identifier, Object[] args) at Microsoft.AspNetCore.Components.Server.Circuits.RemoteNavigationManager.<>c__DisplayClass14_0.<g__RefreshAsync|0>d.MoveNext()

surayya-MS commented 10 months ago

A usability issue in the same area:

If you do NavigationManager.NavigateTo(Nav.Uri) while enhanced nav is enabled, it unconditionally pushes a new history entry, even if it's the same as where you already are. That may be OK for NavigateTo but it means it's not really suitable for implementing post/redirect/get.

There needs to be some way to do post/redirect/get without pushing a new history entry, otherwise "back" then doesn't work as you'd expect. We should check that NavigationManager.Refresh can be fixed to force the enhanced load but without pushing a new history entry.

@SteveSandersonMS would it be an acceptable behavior if in case when form is enhance and url doesn't change, then do not push new history entry? So both NavigationManager.Refresh() and NavigationManager.NavigateTo(NavigationManager.Uri) will not push new history entry

SteveSandersonMS commented 10 months ago

So both NavigationManager.Refresh() and NavigationManager.NavigateTo(NavigationManager.Uri) will not push new history entry

I see how that may be easier to implement and certainly in the great majority of cases is a reasonable behavior. However if we're proposing to make this change in a .NET 8 patch, we may need to be a bit more risk-averse, since this would also be a change of behavior for all existing interactive-mode apps.

How practical would it be to limit the new behavior to Refresh only? I guess it means we have to supply an extra flag in the JS interop call, but since we already do pass flags like that to control push-vs-replace, maybe it's not too problematic to do it.

surayya-MS commented 10 months ago

I was able to repro the bug.

Interestingly, if you choose Blazor Web App with Interactivity Auto instead of None, and try same thing then the error message is different. The error doesn't show on the page but in terminal:

Microsoft.AspNetCore.Components.Server.Circuits.RemoteNavigationManager[5] Failed to refresh System.ArgumentNullException: Value cannot be null. (Parameter 'jsRuntime') at System.ArgumentNullException.Throw(String paramName) at System.ArgumentNullException.ThrowIfNull(Object argument, String paramName) at Microsoft.JSInterop.JSRuntimeExtensions.InvokeVoidAsync(IJSRuntime jsRuntime, String identifier, Object[] args) at Microsoft.AspNetCore.Components.Server.Circuits.RemoteNavigationManager.<>c__DisplayClass14_0.<g__RefreshAsync|0>d.MoveNext()

Thanks, @MackinnonBuck, for pointing out the bug https://github.com/dotnet/aspnetcore/issues/51482