Skip to content

Conversation

SteveSandersonMS
Copy link
Member

@SteveSandersonMS SteveSandersonMS commented Nov 21, 2023

Support NavigateTo when enhanced nav is disabled

Makes the NavigateTo API work even if enhanced nav is disabled via config.

Description

By default, Blazor apps have either enhanced nav or an interactive router . In these default cases, the NavigateTo API works correctly.

However there's also an obscure way to disable both of these via config. It's niche, but it's supported, so the rest of the system should work with that. Unfortunately NavigateTo assumes that either enhanced nav or an interactive router will be enabled and doesn't account for the case when neither is.

Fixes #51636

Customer Impact

Without this fix, anyone who uses the ssr: { disableDomPreservation: true } config option will be unable to use the NavigateTo API, as it will do nothing. This behavior isn't desirable.

Regression?

  • Yes
  • No

No because existing code can't use ssr: { disableDomPreservation: true } as the option didn't exist prior to .NET 8.

Someone else might argue that it's a regression in the sense that, if you're migrating existing code to use newer .NET 8 patterns (and are using disableDomPreservation for some reason, even though you wouldn't normally), your existing uses of NavigateTo could stop working. That's not how we normally define "regression" but I'm trying to give the fullest explanation.

Risk

  • High
  • Medium
  • Low

The fix explicitly retains the old code path if you're coming from .NET 7 or earlier (i.e., if you are using blazor.webassembly/server/webview.js. The fixed code path is only applied in blazor.web.js, so it should not affect existing apps that are simply moving to the net8.0 TFM without other code changes.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Nov 21, 2023
@ghost ghost added this to the 8.0.x milestone Nov 21, 2023
@ghost
Copy link

ghost commented Nov 21, 2023

Hi @SteveSandersonMS. If this is not a tell-mode PR, please make sure to follow the instructions laid out in the servicing process document.
Otherwise, please add tell-mode label.

@SteveSandersonMS SteveSandersonMS force-pushed the stevesa/fix-51636-navigateto-without-enhanced-nav branch from ca911c2 to 634550a Compare November 21, 2023 19:24
@Body
<main>
@Body
</main>
Copy link
Member Author

@SteveSandersonMS SteveSandersonMS Nov 21, 2023

Choose a reason for hiding this comment

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

This change is needed to work around a limitation in the DOM diffing (can't merge two elements of which only one is a LogicalElement). Doesn't affect the validity of the scenario, but doing this allows the E2E test to make stricter and more interesting assertions about element preservation.

// In blazor.web.js, we explicitly recognize the case where you have neither an interactive nor enhanced SSR router
// attached, and then handle Blazor.navigateTo by doing a full page load because that's more useful (issue #51636).
return Blazor._internal.isBlazorWeb ? 'serverside-fullpageload' : 'clientside-router';
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Previously we just had a boolean condition "shouldUseClientSideRouting". However that was misleading because there are really three cases (client-side routing, enhanced nav, and neither). By changing this boolean condition to one with all three possibilities, it forces other code to remember and account for the "neither" case.

@SteveSandersonMS SteveSandersonMS added the Servicing-consider Shiproom approval is required for the issue label Nov 22, 2023
@ghost
Copy link

ghost commented Nov 22, 2023

Hi @SteveSandersonMS. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

@SteveSandersonMS
Copy link
Member Author

More notes for PR reviewers:

This is actually going to be an intentional change of behavior going from blazor.server/webview/webassembly.js to blazor.web.js, but only in the super niche case where you don't have an interactive router and you completely disable enhanced nav in Blazor.start(...) and then are trying to create a custom new kind of router.

  • Before: calling Blazor.navigateTo would only update the NavigationManager state on the .NET side but would not do any actual navigation. That was reported as a bug in NavigateTo doesn't work in Blazor 8 RC2 when enhanced navigation is disabled #51636, but theoretically someone might actually want that behavior (maybe they are implementing a custom interactive router, not using the built-in one, and have forgotten to call EnableNavigationInterceptionAsync to register their custom interactive router).
  • After: calling Blazor.navigateTo when there's no interactive router or enhanced nav will cause a full page load

For back-compat with .NET 7 upgrades even in this very niche case, I've retained the old behavior on blazor.server/webview/webassembly.js, and only applied the fix in blazor.web.js. The other reason to do this is that some of our E2E tests intentionally use this technique as a way to test NavigationManager without involving Router.

@SteveSandersonMS SteveSandersonMS marked this pull request as ready for review November 22, 2023 16:30
@SteveSandersonMS SteveSandersonMS requested a review from a team as a code owner November 22, 2023 16:30
@SteveSandersonMS SteveSandersonMS changed the title Fix #51636 (NavigateTo without enhanced nav) Support NavigateTo when enhanced nav is disabled Nov 22, 2023
@mkArtakMSFT mkArtakMSFT added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Nov 23, 2023
@ghost
Copy link

ghost commented Nov 23, 2023

Hi @SteveSandersonMS. This PR was just approved to be included in the upcoming servicing release. Somebody from the @dotnet/aspnet-build team will get it merged when the branches are open. Until then, please make sure all the CI checks pass and the PR is reviewed.

@mkArtakMSFT mkArtakMSFT merged commit b445dd6 into release/8.0 Nov 23, 2023
@mkArtakMSFT mkArtakMSFT deleted the stevesa/fix-51636-navigateto-without-enhanced-nav branch November 23, 2023 00:20
@ghost ghost modified the milestones: 8.0.x, 8.0.1 Nov 23, 2023
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 Servicing-approved Shiproom has approved the issue
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants