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.43k stars 10.02k forks source link

[Blazor] API for enhanced page refresh #50014

Closed MackinnonBuck closed 1 year ago

MackinnonBuck commented 1 year ago

Background and Motivation

Blazor customers may want to bypass interactive routing during programmatic navigations, falling back to server-side routing using enhanced navigation. This is particularly useful if the customer knows that SSR'd content needs to be refreshed. See https://github.com/dotnet/aspnetcore/issues/49414 for more info.

PR: https://github.com/dotnet/aspnetcore/pull/50012 #50068

Proposed API

namespace Microsoft.AspNetCore.Components;

public readonly struct NavigationOptions
{
+    public bool PreferEnhancedNavigation { get; init; }
}

namespace Microsoft.AspNetCore.Components.Routing;

public interface INavigationInterception
{
+    Task DisableNavigationInterceptionAsync()
+        => Task.CompletedTask;
}

Edit: See updated proposed API here

Usage Examples

Attempt an enhanced navigation, falling back to client-side routing if enhanced navigation is not available:

var uri = ...
NavigationManager.NavigateTo(uri, new NavigationOptions()
{
    PreferEnhancedNavigation = true,
});

Attempt an enhanced navigation, falling back to a full page reload if enhanced navigation is not available:

var uri = ...
NavigationManager.NavigateTo(uri, new NavigationOptions()
{
    PreferEnhancedNavigation = true,
    ForceLoad = true,
});

Alternative Designs

1. A simpler API that only supports the "enhaced page refresh" scenario

namespace Microsoft.AspNetCore.Components;

public abstract class NavigationManager
{
+    public void EnhancedRefresh();
}

This looks cleaner for what I anticipate will be the most common use of this new feature, which is performing an enhanced SSR for the current page. However, it doesn't allow bypassing the interactive router for navigations to other URLs.

2. Alternate names for PreferEnhancedNavigation

Risks

None.

ghost commented 1 year ago

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

SteveSandersonMS commented 1 year ago

Glad to see this going forwards! Thanks for thinking this through.

I agree that the naming is a bit of a challenge here because it's tough to fit it alongside the existing ForceLoad bool flag. Adding a second bool flag inevitably means there are four combinations of flag values, but it's unclear there really are four different levels of "forcedness" or four different useful fallback strategies.

For example, in the proposal here, PreferEnhancedNavigation = true, ForceLoad = false is a really strange combination as defined (use enhanced, else interactive, else full-load). I don't know why anyone would want to do that.

Today we only have two cases: Default (use interactive, else enhanced, else full-load) and Full (use full-load only), as controlled by the ForceLoad flag. For the new feature I could anticipate three primary useful scenarios:

  1. Default (use interactive, else enhanced, else full-load)
  2. SSR update (use enhanced, else full-load)
  3. Full (use full-load only)

This doesn't include the "use enhanced, else interactive, else full-load" strategy, since I don't know what that is good for.

A simpler API that only supports the "enhaced page refresh" scenario This looks cleaner for what I anticipate will be the most common use of this new feature, which is performing an enhanced SSR for the current page. However, it doesn't allow bypassing the interactive router for navigations to other URLs.

That is a pretty appealing simplifications and bypasses the design problems above. I'll take your starting point here and propose a specific possibility:

namespace Microsoft.AspNetCore.Components;

public abstract class NavigationManager
{
+    public void Reload(bool forceLoad = false);
}

This has the benefit of avoiding the term "enhanced" (or "soft" or whatever) and focusing only on the actual outcome. The behavior would be:

Also this uses the term Reload instead of Refresh for parity with JavaScript's location.reload. Arguably we could not have the forceLoad parameter at all, but having it clarifies that simply calling Reload on its own doesn't necessarily cause a full-page load (the outcome depends on whether interactive or enhanced nav is present).

So overall, I think the key design question we have to settle is if there is any important case for bypassing interactive nav while moving to a new URL via enhanced nav. I don't have a good use case in mind for that since I think if someone is using dynamic SSR content they want to update, they wouldn't be using an interactive router. And if a clear need does emerge, this Reload API won't clash with any future "move to a different URL" APIs we might later add for that.

What do you think?

MackinnonBuck commented 1 year ago

So overall, I think the key design question we have to settle is if there is any important case for bypassing interactive nav while moving to a new URL via enhanced nav. I don't have a good use case in mind for that since I think if someone is using dynamic SSR content they want to update, they wouldn't be using an interactive router.

I agree. It definitely seems like the most common use for a feature like this would be refreshing SSR'd content on the current page rather than bypassing an interactive router. And that's a good point that adding a new Reload API doesn't clash with changes to other APIs like NavigationManager.NavigateTo(), so I think it's probably safest to narrow the scope of this API a bit. That way we can collect more customer feedback about what the scenarios are we need to support.

The behavior would be:

  • If forceLoad = false,
    • If interactive routing is available, this simply triggers LocationChanged and does nothing else
    • Else if enhanced nav is available, it does an enhanced nav for the current URL
    • Else it does a full-page reload for the current URL

This seems reasonable. Would we want to support the scenario where even if an interactive router is present, an enhanced refresh can be performed? That specific scenario is what https://github.com/dotnet/aspnetcore/pull/50012 was meant to provide. After the change in https://github.com/dotnet/aspnetcore/pull/49768, it's already possible to come close to the behavior you described (minus the final "else do a full page reload") by doing:

NavigationManager.NavigateTo(NavigationManager.Uri, replace: true);

Should we change this slightly to:

?

SteveSandersonMS commented 1 year ago

Would we want to support the scenario where even if an interactive router is present, an enhanced refresh can be performed?

I think we'd at least need one or two convincing examples of cases where someone would want to do so. Not yet sure what they would be, or if they exist!

Should we change this slightly to:

Flipping the priority order so it goes (use enhanced, else interactive, else full reload) is a bit confusing to me since I still don't know a scenario where someone would want to do it. Reducing it to (use enhanced, else full reload) would somewhat make sense since the desire is to refresh SSR content (which interactive routing never does), but in the case where someone is doing traditional Blazor Server/WebAssembly this would reduce to "always do full reload" which serves no obvious purpose when it can easily be done by other means.

SteveSandersonMS commented 1 year ago

Ultimately I don't have a good sense of why someone would be doing this at all if they have an interactive router, which makes it difficult to design an API for that scenario!

MackinnonBuck commented 1 year ago

Ultimately I don't have a good sense of why someone would be doing this at all if they have an interactive router, which makes it difficult to design an API for that scenario!

Yeah, maybe the "when an interactive router is present" part of this feature is a bit to niche to prioritize then. For the scenario where an interactive router is not present, what do you think about suggesting that customers do:

NavigationManager.NavigateTo(NavigationManager.Uri, replace: true);

...if they want to trigger an "enhanced refresh"? This just wouldn't support falling back on a full page reload when enhanced navigation isn't available. Maybe that limitation is enough of a reason to introduce a new API that has a fallback to a full page reload.

If we go with a new API, I wonder if renaming Reload(bool forceLoad = false) to Refresh(bool forceLoad = false) would make sense. My thinking is that the term "reload" might seem to imply a full page reload, so it might not be clear what the difference is between Reload(true) and Reload(false). It seems to me like an "enhanced reload" is a Blazor-specific concept so therefore deserves a different, less ambiguous term. I don't have a very strong opinion on this though.

SteveSandersonMS commented 1 year ago

For the scenario where an interactive router is not present, what do you think about suggesting that customers do NavigationManager.NavigateTo(NavigationManager.Uri, replace: true);

That's sort of OK, but if we think this is a mainstream scenario, having a specific API for it really helps (otherwise people will think they are relying on a weird hack). I'd prefer the API to exist, especially when it also solves the "what if enhanced nav is not enabled" issue.

I don't mind as much whether it's called Reload or Refresh and think they both adequately convey what goes on. I slightly lean towards Reload still since it's clearer that it's about refetching the whole page, as opposed to somehow "refreshing" the navigation manager itself in some way (e.g., to update the route table, or some other misconception). It could even be called ReloadPage or RefreshPage to hammer this point home, but location.reload has never confused anyone.

Update: We discussed this separately and came to a different conclusion, below.

MackinnonBuck commented 1 year ago

Updated proposed API

namespace Microsoft.AspNetCore.Components;

public abstract class NavigationManager
{
+    public void Refresh();
}

Refresh() always performs an enhanced navigation if possible. Otherwise, a full page reload is performed.

If the customer wants to perform a full page reload even when enhanced navigation is available, they can use the existing NavigationManager.NavigateTo() API with the ForceLoad option specified as true.

halter73 commented 1 year ago

This looks a lot like the original alternative design for EnhancedRefresh(). Do we think just Refresh() is clear enough without the "Enhanced" part even though there's no forceLoad parameter?

Also, the alternative design notes mention that "it doesn't allow bypassing the interactive router for navigations to other URLs." Is there a workaround? Why do we think people will more likely want to do an enhanced refresh of the current page than want to do an enhanced navigation to another page?

Overall, I think this is okay as long as it's well documented and we don't add enhanced navigation to other pages in the future. If we do add enhanced navigation to other pages, having a special method for enhance refresh would be redundant, right?

MackinnonBuck commented 1 year ago

This looks a lot like the original alternative design for EnhancedRefresh(). Do we think just Refresh() is clear enough without the "Enhanced" part even though there's no forceLoad parameter?

Hmm. Naming it EnhancedRefresh() might also be misleading because a full refresh could still happen if enhanced navigation isn't enabled. I'm not sure there's another name that accurately conveys this detail, so I'm wondering if it's best that those subtleties stay in the method's XML docs rather than the method name itself.

That said, this problem might be mitigated if we were to add a bool forceReload = false parameter. It would make more visible the fact that the default behavior doesn't necessitate reloading the page, and it might make things less confusing for those who discover this method while looking for a way to do a full page reload.

Also, the alternative design notes mention that "it doesn't allow bypassing the interactive router for navigations to other URLs." Is there a workaround? Why do we think people will more likely want to do an enhanced refresh of the current page than want to do an enhanced navigation to another page?

There's not a clear scenario for wanting to do an enhanced navigation to another page while an interactive router is in use. In fact, there's not really even a known use case for wanting to do an "enhanced refresh" when an interactive router is in use, but it seemed to make more sense to implement this feature without regard to interactive routing rather than add a special case for "if the interactive router is available, do \."

Enhanced navigation to other routes still happens when an interactive router is not present, which will be the most common case when using Blazor with SSR.

Overall, I think this is okay as long as it's well documented and we don't add enhanced navigation to other pages in the future. If we do add enhanced navigation to other pages, having a special method for enhance refresh would be redundant, right?

I think the question should be "If we do add enhanced navigation to other pages when an interactive router is present, having a special method for enhance refresh would be redundant, right?" Because enhanced navigation to other pages already happens with NavigationManager.NavigateTo() when server-side routing is used.

But I think even if we were to add a way to bypass the interactive router during navigation, having a "Refresh" API is still convenient because it would be a lot shorter than, for example:

NavigationManager.NavigateTo(NavigationManager.Uri, new NavigationOptions { BypassInteractiveRouter = true });
SteveSandersonMS commented 1 year ago

That said, this problem might be mitigated if we were to add a bool forceReload = false parameter.

I'm fine either way on that. I do see the value in adding the parameter even though it's not strictly required (because a forced load can be achieved through other APIs).

even if we were to add a way to bypass the interactive router during navigation, having a "Refresh" API is still convenient

Agreed

So in summary it sounds like we continue with having just Refresh as described here. As for whether or not there's also an overload that takes forceReload - it seems viable either way - so do you want to decide based on what you have time to implement, @MackinnonBuck?

MackinnonBuck commented 1 year ago

I decided to add an optional forceReload parameter. The new API looks like this:

namespace Microsoft.AspNetCore.Components;

public abstract class NavigationManager
{
+    /// <summary>
+    /// Refreshes the current page via request to the server.
+    /// </summary>
+    /// <remarks>
+    /// If <paramref name="forceReload"/> is <c>true</c>, a full page reload will always be performed.
+    /// Otherwise, the response HTML may be merged with the document's existing HTML to preserve client-side state,
+    /// falling back on a full page reload if necessary.
+    /// </remarks>
+    public void Refresh(bool forceReload = false);
}

@halter73 What do you think?

halter73 commented 1 year ago

Based on our discussion here and over email, I think we're good.

API Approved!

MackinnonBuck commented 1 year ago

Completed in https://github.com/dotnet/aspnetcore/pull/50068