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

[release/8.0] [Blazor] Dynamically toggle navigation interception #50564

Closed
wants to merge 1 commit into from

Conversation

MackinnonBuck
Copy link
Member

Dynamically toggle navigation interception

Disables navigation interception when the interactive router gets removed from the page.

Description

If an interactive router gets dynamically removed, server-side routing doesn't take over again, and each navigation updates the address bar without changing any content on the page.

This PR makes it so that navigation interception gets disabled when the router gets removed, which allows server-side routing to work again.

Fixes #50069

@MackinnonBuck MackinnonBuck added the area-blazor Includes: Blazor, Razor Components label Sep 6, 2023
@MackinnonBuck MackinnonBuck requested a review from a team as a code owner September 6, 2023 22:54
@MackinnonBuck
Copy link
Member Author

Still need to finish E2E tests.

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Sep 11, 2023

We should discuss if this is still required given the broader changes to enhanced navigation in #50551

My guess is the main scenario where "interactive router gets removed" was navigating from Blazor to non-Blazor pages. In other cases, it's not obvious to me what reasonable app pattern would result in an interactive router getting removed. The issue #50069 doesn't say why this is typically going to happen. In the past we didn't support dynamically removing an interactive router, so don't think we have to add that support for its own sake, ignoring the "enhanced-navigate away from Blazor completely" case.

If that was the only scenario, we no longer have to account for it. As of #50551, enhanced nav will not apply when navigating to non-Blazor endpoints - it will automatically perform a full-page load. Can simplify things a lot by considering that sufficient?

@javiercn
Copy link
Member

My guess is the main scenario where "interactive router gets removed" was navigating from Blazor to non-Blazor pages. In other cases, it's not obvious to me what reasonable app pattern would result in an interactive router getting removed. The issue #50069 doesn't say why this is typically going to happen. In the past we didn't support dynamically removing an interactive router, so don't think we have to add that support for its own sake, ignoring the "enhanced-navigate away from Blazor completely" case.

Could there be cases here where you are only including the router conditionally on SSR? Like an island that uses a router internally to represent state (for a tabbed UI) for example?

@MackinnonBuck
Copy link
Member Author

Could there be cases here where you are only including the router conditionally on SSR? Like an island that uses a router internally to represent state (for a tabbed UI) for example?

This was exactly the type of case I was thinking of. Maybe an interactive router is present, you do a NavigationManager.Refresh(), and the interactive router gets removed via SSR update. I don't anticipate that this is going to be an extremely common scenario though, so if you think this is something that can wait for .NET 9, I wouldn't have a problem with that.

@SteveSandersonMS @javiercn

@SteveSandersonMS
Copy link
Member

Could there be cases here where you are only including the router conditionally on SSR? Like an island that uses a router internally to represent state (for a tabbed UI) for example?

Theoretically yes, but it's not something I've encountered in the wild. And doing so for a reusable component like "tab set" wouldn't work in many cases since as soon as someone starts with this pattern, they have to be careful to avoid ever having two such islands at the same time, since the history stack is global per-tab and we don't support having multiple concurrent routers.

I don't anticipate that this is going to be an extremely common scenario though, so if you think this is something that can wait for .NET 9, I wouldn't have a problem with that.

Agreed. I wouldn't prioritize this for .NET 8, and would rather leave the framework more flexibility to decide if/how to support this in the future. Does that sound OK?

@MackinnonBuck
Copy link
Member Author

Agreed. I wouldn't prioritize this for .NET 8, and would rather leave the framework more flexibility to decide if/how to support this in the future. Does that sound OK?

Yep, totally ok with me! I'll close this PR for now.

@wtgodbe wtgodbe added this to the 8.0.0 milestone Oct 3, 2023
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Oct 3, 2023
@MackinnonBuck MackinnonBuck deleted the mbuck/disable-nav-interception branch November 5, 2024 21:51
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 area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants