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

[Blazor] API for enhanced page refresh #50014

Closed
MackinnonBuck opened this issue Aug 10, 2023 · 14 comments
Closed

[Blazor] API for enhanced page refresh #50014

MackinnonBuck opened this issue Aug 10, 2023 · 14 comments
Labels
api-approved API was approved in API review, it can be implemented area-blazor Includes: Blazor, Razor Components
Milestone

Comments

@MackinnonBuck
Copy link
Member

MackinnonBuck commented Aug 10, 2023

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 #49414 for more info.

PR: #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

  • PreferSoftNavigation
    • The term "enhanced" doesn't appear in our API surface yet, afaik. Another term (like "soft") might be clearer to customers.
  • BypassInteractiveRouter
    • Changes the meaning of the feature slightly. This name seems to imply it would perform enhanced navigation if possible, falling back to a full page reload if either ForceLoad is true or enhanced navigation is not available. It would not allow the customer to fall back to interactive routing if enhanced navigation could not be performed. We might need to think more deeply about whether falling back to interactive routing is ever desirable, or if it would always be preferable to fall back to a full page reload.

Risks

None.

@MackinnonBuck MackinnonBuck added area-blazor Includes: Blazor, Razor Components api-suggestion Early API idea and discussion, it is NOT ready for implementation api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Aug 10, 2023
@ghost
Copy link

ghost commented Aug 11, 2023

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:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Aug 14, 2023

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:

  • 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
  • If forceLoad = true
    • Regardless of anything else, does a full-page reload for the current URL

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
Copy link
Member Author

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 #50012 was meant to provide. After the change in #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:

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

?

@SteveSandersonMS
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

MackinnonBuck commented Aug 14, 2023

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
Copy link
Member

SteveSandersonMS commented Aug 14, 2023

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
Copy link
Member Author

MackinnonBuck commented Aug 14, 2023

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.

@MackinnonBuck MackinnonBuck changed the title Allow enhanced navigation when an interactive router is present [Blazor] API for enhanced page reload Aug 14, 2023
@MackinnonBuck MackinnonBuck changed the title [Blazor] API for enhanced page reload [Blazor] API for enhanced page refresh Aug 14, 2023
@halter73
Copy link
Member

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
Copy link
Member Author

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 <some other thing>."

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
Copy link
Member

SteveSandersonMS commented Aug 16, 2023

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?

@danroth27 danroth27 added this to the 8.0-rc1 milestone Aug 16, 2023
@MackinnonBuck
Copy link
Member Author

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 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Aug 17, 2023
@halter73
Copy link
Member

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

API Approved!

@MackinnonBuck
Copy link
Member Author

Completed in #50068

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

No branches or pull requests

4 participants