Added additional boolean parameter "replace" to NavigationManager.NavigateTo

[MariovanZeist]

Added optional boolean "replace" to Navigationmanager.NavigateTo

setting boolean to true will replace the uri in the browser history, instead of the default action that pushes the new uri onto the browsers history stack.

The core navigation infrastructure already has a functional "replace" parameter, but this parameter was not publicly exposed.

Addresses #25540

---

API review notes

This section is by @SteveSandersonMS and refers to the outcome I'm proposing with my suggestions below, rather than the current implementation.

The proposal is to add the following APIs in Microsoft.AspNetCore.Components, also showing selected existing APIs for context:

namespace Microsoft.AspNetCore.Components
{
    public abstract class NavigationManager
    {
        public void NavigateTo(string uri, bool forceLoad = false) { /* Calls NavigateToCore(string, bool) */ }
+       public void NavigateTo(string uri, NavigationOptions options) { /* Calls NavigateToCore(string, NavigationOptions) */ }

        protected abstract void NavigateToCore(string uri, bool forceLoad);
+       protected virtual void NavigateToCore(string uri, NavigationOptions options);
    }

+   [Flags]
+   public enum NavigationOptions
+   {
+       ForceLoad = 1,
+       ReplaceHistoryEntry = 2,
+   }
}

The main compromise in this design is around back-compatibility. I'm proposing we prioritise keeping people's existing unit test code working. If we didn't have to do that, we'd outright replace the existing NavigateToCore method with the new one, or at least obsolete the older one. However I don't see a way to obsolete the older one, since NavigateTo(string, bool) has to continue calling it, otherwise that will break anyone who has an existing mock or fake of NavigationManager.

I think the end result of this compromise is totally fine, in that for application developers, there's no drawback at all. The only people who have an extra method to overload is us (or other platform authors).

[SteveSandersonMS]

Thanks again for this, @MariovanZeist!

I hope you don't mind the large collection of suggestions I'm posting here. The core point I'm suggesting is that we can and should do this without it being a breaking change to people's existing unit tests. Everyone who has existing fake/mock implementations of NavigationManager should be able to keep their existing code working. We can achieve this by leaving the existing NavigateToCore alone, and having the new overload act as an independent parallel code path.

I know that makes it less of a clean change, and doesn't immediately get us to a world where people implementing NavigationManager only have a single NavigateToCore to override. However the only people who have to override both overloads will be framework/platform authors, and that's the sort of thing platform authors can figure out just fine. Application developers only subclass NavigationManager for unit tests, and in that case, they will only need to override the overload that their own unit test is actually using, so this will make sense.

Is it essential that, in the long term, we find some way to obsolete the (string, bool) overload? It's not clear to me that is essential. It more seems like a nice-to-have for framework developers, but won't make much (perhaps any) difference to application developers.

[ghost]

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]

@MariovanZeist Sorry for the disruption, but we had an API review meeting in which we realised that NavigationOptions should really be a [Flags] enum instead of struct or class.

Reasoning: a flags enum is sufficient for current use cases, extensible in the future if we end up with more flags, avoids the problems of both struct (can't be extended while retaining binary compatibility) and class (causes an allocation on every call), and if we really do need more complex options in the future, the flags enum can become part of whatever options class we end up using.

Hope this is OK!

[MariovanZeist]

Sorry @javiercn and @pranavkm You were added because a file had their encoding changed. I reverted that change.

[MariovanZeist]

Applied remarks.
This makes the PR a lot cleaner and with fewer changes.
Making the NavigationOptions an Enum instead of a class is a better solution, especially as these are all just flags.

[MariovanZeist]

@SteveSandersonMS Thanks for the review. I have implemented your suggestions.
There is 1 issue that we need to resolve before we merge. That is the pragma I added.
Review comment is added to source code.

[SteveSandersonMS]

This is great, @MariovanZeist. Thanks very much! I agree the current state of this PR is very good. It's a nice clean simple change.

The only thing missing before we can merge is test coverage. If you wanted, you could add unit test coverage for each of the NavigateTo overloads into the existing NavigationManagerTest.cs, showing that they translate into the expected NavigateToCore calls.

More importantly though, we need coverage in the E2E tests to show that the new APIs actually do what they claim to do in real browsers. In src\Components\test\E2ETest\Tests\RoutingTest.cs, you'll find existing E2E tests called CanNavigateProgrammatically and CanNavigateProgrammaticallyWithForceLoad. We'll need to add at least two further tests in a similar style:

• One called CanNavigateProgrammaticallyWithReplaceHistoryEntry, which I guess should do a "replace" navigation, then trigger a "back", and verify that we end up on the page before the one that redirected, and that all of this was client-side navigation (see the existing test cases for how to verify that)
• One called CanNavigateProgrammaticallyWithForceLoadAndReplaceHistoryEntry, which would be the same except we should verify that the initial navigation was a full-page load (see the existing test cases for how to verify that)

Is this possible? Thanks again!

[MariovanZeist]

@SteveSandersonMS Besides the above-mentioned tests, I added a 3rd. (CanNavigateProgrammaticallyValidateNoReplaceHistoryEntry)This one checks if you do not specify ReplaceHistoryState, we indeed do not replace the history state. (It also checks if the new overload NavigateTo(string uri) works as intended. This might be a little overkill, but better to have extra tests than no tests I reckon.

[MariovanZeist]

@SteveSandersonMS During the testing of this PR I came across an issue that is related but not caused by this PR. (To be sure I am going to verify if the same problems occur on master)
The issue is related to the ForceLoad flag and I see different behaviors across browsers. From working as intended, Reconnect issues, and downright crashing in js (getting the Reload option in the yellow bar)
I am testing Chrome / Edge and Firefox.
Might be related to browser caching, or a known issue. Ignore for now.

[SteveSandersonMS]

This logic looks wrong to me. What happens if isWithinBaseUriSpace(absoluteUri) is false, force is false, and replace is true?

What I'd expect is: since this is an external navigation, the force param is irrelevant, and the replace flag means we should be calling location.replace.

What the logic appears to do is call history.replaceState, which won't produce a useful behavior because it's only doing HTML5-style client-side navigation and won't trigger an actual external page load.

Here's the full truth table I'd expect (ignoring the Force-loading the same URL you're already on special case):

isWithinBaseUriSpace(absoluteUri)	Force	Replace	Expected behavior
false	false	false	location.href = url (append external page load to history)
false	false	true	location.replace (replace with external page load on history)
false	true	false	location.href = url (append external page load to history)
false	true	true	location.replace (replace with external page load on history)
true	false	false	performInternalNavigation(absoluteUri, false, false) (calls history.pushState)
true	false	true	performInternalNavigation(absoluteUri, false, true) (calls history.replaceState)
true	true	false	location.href = url (append external page load to history)
true	true	true	location.replace (replace with external page load on history)

Do you agree with this list? Could you check how it corresponds to the implementation? I think the only combination that's currently wrong is the one I mentioned at the top of this comment.

I suspect that before this PR, the logic here was already wrong in that it didn't handle replace without also force. That was OK before because it wasn't really proper public API. But since this is becoming proper public API now, we should make sure all the combinations of parameters work correctly.

[SteveSandersonMS]

Suggested change

[SteveSandersonMS]

@MariovanZeist This looks really good to me. Thanks for submitting it!

I think there's just one more implementation issue to resolve and then we should be ready to merge this.

[ghost]

Hi MariovanZeist.
It seems you haven't touched this PR for the last two weeks. To avoid accumulating old PRs, we're marking it as stale. As a result, it will be closed if no further activity occurs within 4 days of this comment. You can learn more about our Issue Management Policies here.

[SteveSandersonMS]

Continuing this work at #33751