Navigate with "replace" param #33751

SteveSandersonMS · 2021-06-22T12:18:31Z

This is a continuation of the work at #25752

It's a surprisingly involved change for what seems like just adding an extra bool flag. Mainly this is because it happens to be on one of the extensibility seams so we have to work hard to ensure it's nonbreaking for existing NavigationManager subclasses (e.g., that people have made for tests).

The approach taken here is:

Existing calls to NavigateTo(string, bool=false) will continue to work, because of carefully adding a combination of overloads that retains both source and binary compatibility
Existing NavigationManager subclasses that override abstract void NavigateToCore(string, bool) will continue to work because the framework continues to call that as much as it can (i.e., whenever the caller doesn't specify replace=true).
New NavigationManager subclasses no longer need to override abstract void NavigateToCore(string, bool) - they can do if they want, but instead, it's sufficient to override virtual void NavigateToCore(string, NavigationOptions) since the new base class implementation of (string, bool) calls into it.

There is also a bunch of refactoring on the JS side because even though the previous code had both force and replace flags, it actually didn't support them both being true at the same time. Supporting this extra case made the existing code hard to read so I've restructured it for simplicity. AFAIK this should not result in any back-compat loss, even in extreme edge cases of unrecognized URL formats, because of retaining the distinction between url and absoluteUrl in the code paths.

SteveSandersonMS · 2021-06-23T10:13:45Z

src/Components/Components/src/NavigationOptions.cs

+        /// If false, appends the new entry to the history stack.
+        /// </summary>
+        public bool ReplaceHistoryEntry { get; init; }
+    }


In an earlier edition of this PR, NavigationOptions was a flags enum. I've changed it to a full-fledged struct because I don't want to get into this situation again in the future where we need to add some other option and have to multiply out the number of overloads and obscure rules for what methods you have to implement in subclasses.

With a struct, even if we need new options of type string, or rules about which combinations of options are legal to use simultaneously, it should be possible to model that (e.g., as different constructor overloads).

SteveSandersonMS · 2021-06-23T10:15:18Z

src/Components/Components/src/PublicAPI.Unshipped.txt

@@ -2,6 +2,8 @@
 *REMOVED*static Microsoft.AspNetCore.Components.ParameterView.FromDictionary(System.Collections.Generic.IDictionary<string!, object!>! parameters) -> Microsoft.AspNetCore.Components.ParameterView
 *REMOVED*virtual Microsoft.AspNetCore.Components.RenderTree.Renderer.DispatchEventAsync(ulong eventHandlerId, Microsoft.AspNetCore.Components.RenderTree.EventFieldInfo! fieldInfo, System.EventArgs! eventArgs) -> System.Threading.Tasks.Task!
 *REMOVED*readonly Microsoft.AspNetCore.Components.RenderTree.RenderTreeEdit.RemovedAttributeName -> string!
+*REMOVED*Microsoft.AspNetCore.Components.NavigationManager.NavigateTo(string! uri, bool forceLoad = false) -> void
+*REMOVED*abstract Microsoft.AspNetCore.Components.NavigationManager.NavigateToCore(string! uri, bool forceLoad) -> void


Although these show as removed, there should be no back-compat loss (source or binary) because of adding new equivalents. I think our PublicAPI tooling doesn't reflect some details about how adding new optional params.

But if you're code-reviewing this, please check you agree with my claim here :)

javiercn · 2021-06-25T09:21:52Z

src/Components/Components/src/PublicAPI.Unshipped.txt

@@ -29,6 +31,15 @@ Microsoft.AspNetCore.Components.Lifetime.ComponentApplicationLifetime.State.get
 Microsoft.AspNetCore.Components.Lifetime.IComponentApplicationStateStore
 Microsoft.AspNetCore.Components.Lifetime.IComponentApplicationStateStore.GetPersistedStateAsync() -> System.Threading.Tasks.Task<System.Collections.Generic.IDictionary<string!, byte[]!>!>!
 Microsoft.AspNetCore.Components.Lifetime.IComponentApplicationStateStore.PersistStateAsync(System.Collections.Generic.IReadOnlyDictionary<string!, byte[]!>! state) -> System.Threading.Tasks.Task!
+Microsoft.AspNetCore.Components.NavigationManager.NavigateTo(string! uri, Microsoft.AspNetCore.Components.NavigationOptions options) -> void
+Microsoft.AspNetCore.Components.NavigationManager.NavigateTo(string! uri, bool forceLoad = false, bool replace = false) -> void


Do we need this overload?

I think we should force people into using the navigation options directly and set ourselves for success in the future.

Personally I find:

navManager.NavigateTo("something", replace: true);

to be a lot more readable than:

navManager.NavigateTo("something", new NavigationOptions { ReplaceHistoryEntry = true });

However I understand it's always a subjective balance when we consider adding overloads just for convenience. It depends on how universal we think the convenience-overload pattern will be, and whether it will stand the test of time as being the only extra convenience we need, vs something that ends up getting multiplied out into many other overload variants. (I know you know this; just giving context for anyone else reading)

We do need at least to keep the NavigateTo(string, bool) overload for back-compat. To me it felt a bit rough if this other equally-often-used flag was not available in a similar shorthand format.

Shall we leave a final call on this until we get to API review, or do you feel strongly now?

I don't feel strongly about it, it was mainly a suggestion.

javiercn · 2021-06-25T09:26:10Z

src/Components/Web.JS/src/Services/NavigationManager.ts

+// For back-compat, we need to accept multiple overloads
+export function navigateTo(uri: string, options: NavigationOptions): void;
+export function navigateTo(uri: string, forceLoad: boolean): void;
+export function navigateTo(uri: string, forceLoad: boolean, replace: boolean): void;
+export function navigateTo(uri: string, forceLoadOrOptions: NavigationOptions | boolean, replaceIfUsingOldOverload: boolean = false) {


I haven't seen this used in a while in TS 😄.

I believe navigateTo ends up in our internal namespace. The only thing that uses that overload is the auth bits in one place. I'm happy if you update those and avoid having to deal with the special case here, since we can consider this API internal and for our consumption, akin to private reflection.

It's actually not within internal, however it's here

Blazor.navigateTo (the JS API) is very much public and supported. It's the only way to trigger a client-side Blazor navigation from JS code, so is certainly needed.

Yeah, I saw that later. If you want to update the auth package as part of this change to rely on that, It would be nice.

javiercn

Looks great!

SteveSandersonMS · 2021-06-29T15:01:33Z

/azp run

azure-pipelines · 2021-06-29T15:01:58Z

Azure Pipelines successfully started running 2 pipeline(s).

…igateTo So instead of pushing the new uri into the browser history, we replace the uri in the browser history

Modified NavigateTo be source and binary compatible with previous versions See: to https://github.com/dotnet/roslyn/blob/master/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md

!!!IMPORTANT!!!! I have also added a pragma to disable a warning which should NOT be disabled. It's only added to get it reviewed and fixed. #pragma warning disable RS0027 // Public API with optional parameter(s) should have the most parameters amongst its public overloads. Is added to the NavigationManager Before merging this should be addressed.

The forceLoad flag would be ignored if not loading the same Uri

ghost added the area-blazor Includes: Blazor, Razor Components label Jun 22, 2021

SteveSandersonMS mentioned this pull request Jun 22, 2021

Added additional boolean parameter "replace" to NavigationManager.NavigateTo #25752

Closed

SteveSandersonMS commented Jun 23, 2021

View reviewed changes

SteveSandersonMS requested a review from a team June 23, 2021 10:16

SteveSandersonMS marked this pull request as ready for review June 23, 2021 10:16

javiercn reviewed Jun 25, 2021

View reviewed changes

javiercn approved these changes Jun 25, 2021

View reviewed changes

SteveSandersonMS force-pushed the stevesa/navigate-replace branch from 0fd0c05 to 9ee3402 Compare June 29, 2021 14:57

MariovanZeist and others added 18 commits June 30, 2021 13:18

Added additional boolean parameter "replace" to NavigationManager.Nav…

86cbf05

…igateTo So instead of pushing the new uri into the browser history, we replace the uri in the browser history

Changes due to remarks

253481f

Updated PublicApi

dec6753

Modified NavigateTo be source and binary compatible with previous versions See: to https://github.com/dotnet/roslyn/blob/master/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md

Called wrong Exception, fixed MVC test

4434740

Reverted Encoding to version in master. No need for this file to change

fc21398

Avoid RS0027 problem by replacing optional param with extra overload

6f9432f

Implemented PR remarks.

63fecca

When navigateTo was called with both flags set (ForceLoad and Replace)

b4fb11c

The forceLoad flag would be ignored if not loading the same Uri

Added 3 e2e tests

6ece74b

Change options from flags enum to readonly struct

097ebe2

Update logging

0414f1b

Update E2E test code

7687790

Clean up TS-side routing logic

42b8e0d

Tweak

0e5342f

More conservative logic for extra back-compat

2d64856

Cleanups in E2E test code

92cd5b8

Fix unit test

983c10a

Update .js files

4fb201b

SteveSandersonMS force-pushed the stevesa/navigate-replace branch from 7a18e65 to 4fb201b Compare June 30, 2021 12:31

Fixes

a68395e

SteveSandersonMS force-pushed the stevesa/navigate-replace branch from 1adc8cd to a68395e Compare June 30, 2021 13:50

SteveSandersonMS merged commit ed383aa into main Jun 30, 2021

SteveSandersonMS deleted the stevesa/navigate-replace branch June 30, 2021 15:40

ghost added this to the 6.0-preview7 milestone Jun 30, 2021

campersau mentioned this pull request Jul 23, 2021

Blazor: expose 'replace' parameter in NavigationManager.NavigateTo #25540

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Navigate with "replace" param #33751

Navigate with "replace" param #33751

SteveSandersonMS commented Jun 22, 2021 •

edited

Loading

SteveSandersonMS Jun 23, 2021

SteveSandersonMS Jun 23, 2021

javiercn Jun 25, 2021

SteveSandersonMS Jun 25, 2021

javiercn Jun 25, 2021

javiercn Jun 25, 2021

javiercn Jun 25, 2021

SteveSandersonMS Jun 25, 2021

javiercn Jun 25, 2021

javiercn left a comment

SteveSandersonMS commented Jun 29, 2021

azure-pipelines bot commented Jun 29, 2021

Navigate with "replace" param #33751

Navigate with "replace" param #33751

Conversation

SteveSandersonMS commented Jun 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiercn left a comment

Choose a reason for hiding this comment

SteveSandersonMS commented Jun 29, 2021

azure-pipelines bot commented Jun 29, 2021

SteveSandersonMS commented Jun 22, 2021 •

edited

Loading