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] Allow enhanced navigation when an interactive router is present #50012

Closed
wants to merge 1 commit into from

Conversation

MackinnonBuck
Copy link
Member

Allow enhanced navigation when an interactive router is present

Adds a new API to NavigationOptions to prefer enhanced navigation even when an interactive router is present.

Description

This PR makes the following change to NavigationOptions:

namespace Microsoft.AspNetCore.Components;

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

This property influences the behavior of NavigationManager.NavigateTo():

  • If enhanced navigation can be performed, always prefer that, even if NavigationOptions.ForceLoad is true
  • Otherwise (if enhanced navigation cannot be performed):
    • If ForceLoad is true, do a full page reload
    • Otherwise, do an internal navigation using client-side routing
    • This allows the customer to control the fallback behavior of PreferEnhancedNavigation by modifying the ForceLoad property

Furthermore, this PR adds a new DisableNavigationInterception method to INavigationInterception so that when the interactive router gets disposed, enhanced navigation can start intercepting browser-initiated navigations again.

Fixes #49414

@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Aug 10, 2023
@MackinnonBuck
Copy link
Member Author

Opening as a draft to gather initial feedback. Still need to finish E2E tests.

await _jsRuntime.InvokeAsync<object>(Interop.DisableNavigationInterception);
}
catch (JSDisconnectedException)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it OK for us to consume this exception?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. This happens if the browser is no longer available, in which case there's no browser-side state to clean up. In other words, if this exception gets thrown, it means there's no work to do.

@@ -46,6 +47,20 @@ function listenForNavigationEvents(
currentHistoryIndex = history.state?._index ?? 0;
}

async function enableNavigationInterception(uriInDotNet?: string) {
setHasInteractiveRouter(true);
if (uriInDotNet && location.href !== uriInDotNet) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In what scenarios does this happen?

Copy link
Member Author

@MackinnonBuck MackinnonBuck Aug 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an example:

  1. The user is currently on a page with an interactive router
  2. The user navigates to a page without an interactive router, disposing the router and disabling navigation interception
  3. The user continues to click around, all while .NET is not getting notified of location changes (because no interactive router is present)
  4. The user navigates back to a page with an interactive router. Navigation interception gets enabled, but the itneractive router didn't get a chance to detect the navigation that just happened, so the interactive .NET NavigationManager still has the wrong URL and the router will therefore render content for the wrong page (or hit the "not found" route).

@halter73
Copy link
Member

  • If enhanced navigation can be performed, always prefer that, even if NavigationOptions.ForceLoad is true

This sounds unintuitive to me. Wouldn't most people expect ForceLoad to take precedence over PreferEnhancedNavigation? "Force" sounds much more forceful than "Prefer" to me. And they're both on the same options type.

@MackinnonBuck
Copy link
Member Author

MackinnonBuck commented Aug 11, 2023

  • If enhanced navigation can be performed, always prefer that, even if NavigationOptions.ForceLoad is true

This sounds unintuitive to me. Wouldn't most people expect ForceLoad to take precedence over PreferEnhancedNavigation? "Force" sounds much more forceful than "Prefer" to me. And they're both on the same options type.

That's fair. But another way to think of it is that ForceLoad isn't getting ignored - the PreferEnhancedNavigation option just also forces a request to be made to the server, and the ForceLoad option just becomes redundant. But if an enhanced navigation can't be made, then ForceLoad's behavior still applies. The difference is that if the enhanced navigation isn't performed, a full page reload is made.

But I can definitely see that as being confusing. An alternate API I mentioned in #50014 is NavigationOptions.BypassInteractiveRouter, which would basically mean "pretend an interactive router doesn't exist and always use server side routing". So ForceLoad would always do a full page reload rather than just forcing a request to be made to the server. If ForceLoad is not specified, then an enhanced navigation is performed if possible, otherwise a full page reload still gets performed. This approach would not allow the customer to fall back to interactive routing if enhanced navigation could not be performed, but I'm not sure whether that's a realistic use case anyway.

@MackinnonBuck
Copy link
Member Author

Closing in favor of #50068

@ghost
Copy link

ghost commented Aug 14, 2023

Hi @MackinnonBuck. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to programmatically perform an "enhanced page refresh"
3 participants