Make OnNavigateAsync EventCallback and cancel previous navigation (#25011)

captainsafia · web-flow · commit 8fc1419186f6 · 2020-08-19T18:25:24.000-07:00
* Make OnNavigateAsync EventCallback and cancel previous navigation

* Add more tests
diff --git a/src/Components/Components/src/Routing/Router.cs b/src/Components/Components/src/Routing/Router.cs
@@ -73,7 +73,7 @@ static readonly ReadOnlyDictionary<string, object> _emptyParametersDictionary
         /// <summary>
         /// Gets or sets a handler that should be called before navigating to a new page.
         /// </summary>
-        [Parameter] public Func<NavigationContext, Task>? OnNavigateAsync { get; set; }
+        [Parameter] public EventCallback<NavigationContext> OnNavigateAsync { get; set; }
 
         private RouteTable Routes { get; set; }
 
@@ -115,8 +115,7 @@ public async Task SetParametersAsync(ParameterView parameters)
             if (!_onNavigateCalled)
             {
                 _onNavigateCalled = true;
-                await RunOnNavigateWithRefreshAsync(NavigationManager.ToBaseRelativePath(_locationAbsolute), isNavigationIntercepted: false);
-                return;
+                await RunOnNavigateAsync(NavigationManager.ToBaseRelativePath(_locationAbsolute), isNavigationIntercepted: false);
             }
 
             Refresh(isNavigationIntercepted: false);
@@ -206,9 +205,8 @@ internal virtual void Refresh(bool isNavigationIntercepted)
             }
         }
 
-        private async ValueTask<bool> RunOnNavigateAsync(string path, Task previousOnNavigate)
+        internal async ValueTask RunOnNavigateAsync(string path, bool isNavigationIntercepted)
         {
-
             // Cancel the CTS instead of disposing it, since disposing does not
             // actually cancel and can cause unintended Object Disposed Exceptions.
             // This effectivelly cancels the previously running task and completes it.
@@ -217,67 +215,43 @@ private async ValueTask<bool> RunOnNavigateAsync(string path, Task previousOnNav
             // before starting the next one. This avoid race conditions where the cancellation
             // for the previous task was set but not fully completed by the time we get to this
             // invocation.
-            await previousOnNavigate;
+            await _previousOnNavigateTask;
 
-            if (OnNavigateAsync == null)
+            var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
+            _previousOnNavigateTask = tcs.Task;
+
+            if (!OnNavigateAsync.HasDelegate)
             {
-                return true;
+                Refresh(isNavigationIntercepted);
             }
 
             _onNavigateCts = new CancellationTokenSource();
             var navigateContext = new NavigationContext(path, _onNavigateCts.Token);
 
+            var cancellationTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
+            navigateContext.CancellationToken.Register(state =>
+                ((TaskCompletionSource)state).SetResult(), cancellationTcs);
+
             try
             {
-                if (Navigating != null)
-                {
-                    _renderHandle.Render(Navigating);
-                }
-                await OnNavigateAsync(navigateContext);
-                return true;
-            }
-            catch (OperationCanceledException e)
-            {
-                if (e.CancellationToken != navigateContext.CancellationToken)
-                {
-                    var rethrownException = new InvalidOperationException("OnNavigateAsync can only be cancelled via NavigateContext.CancellationToken.", e);
-                    _renderHandle.Render(builder => ExceptionDispatchInfo.Throw(rethrownException));
-                }
+                // Task.WhenAny returns a Task<Task> so we need to await twice to unwrap the exception
+                var task = await Task.WhenAny(OnNavigateAsync.InvokeAsync(navigateContext), cancellationTcs.Task);
+                await task;
+                tcs.SetResult();
+                Refresh(isNavigationIntercepted);
             }
             catch (Exception e)
             {
                 _renderHandle.Render(builder => ExceptionDispatchInfo.Throw(e));
             }
-
-            return false;
-        }
-
-        internal async Task RunOnNavigateWithRefreshAsync(string path, bool isNavigationIntercepted)
-        {
-            // We cache the Task representing the previously invoked RunOnNavigateWithRefreshAsync
-            // that is stored. Then we create a new one that represents our current invocation and store it
-            // globally for the next invocation. This allows us to check inside `RunOnNavigateAsync` if the
-            // previous OnNavigateAsync task has fully completed before starting the next one.
-            var previousTask = _previousOnNavigateTask;
-            var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
-            _previousOnNavigateTask = tcs.Task;
-
-            // And pass an indicator for the previous task to the currently running one.
-            var shouldRefresh = await RunOnNavigateAsync(path, previousTask);
-            tcs.SetResult();
-            if (shouldRefresh)
-            {
-                Refresh(isNavigationIntercepted);
-            }
-
         }
 
         private void OnLocationChanged(object sender, LocationChangedEventArgs args)
         {
             _locationAbsolute = args.Location;
             if (_renderHandle.IsInitialized && Routes != null)
             {
-                _ = RunOnNavigateWithRefreshAsync(NavigationManager.ToBaseRelativePath(_locationAbsolute), args.IsNavigationIntercepted);
+                _ = RunOnNavigateAsync(NavigationManager.ToBaseRelativePath(_locationAbsolute), args.IsNavigationIntercepted);
             }
         }
 
diff --git a/src/Components/Components/test/Routing/RouterTest.cs b/src/Components/Components/test/Routing/RouterTest.cs
@@ -10,9 +10,7 @@
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Logging.Abstractions;
-using Moq;
 using Xunit;
-using Microsoft.AspNetCore.Components;
 
 namespace Microsoft.AspNetCore.Components.Test.Routing
 {
@@ -42,109 +40,61 @@ public async Task CanRunOnNavigateAsync()
         {
             // Arrange
             var called = false;
-            async Task OnNavigateAsync(NavigationContext args)
+            Action<NavigationContext> OnNavigateAsync = async (NavigationContext args) =>
             {
                 await Task.CompletedTask;
                 called = true;
-            }
-            _router.OnNavigateAsync = OnNavigateAsync;
-
-            // Act
-            await _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateWithRefreshAsync("http://example.com/jan", false));
-
-            // Assert
-            Assert.True(called);
-        }
-
-        [Fact]
-        public async Task CanHandleSingleFailedOnNavigateAsync()
-        {
-            // Arrange
-            var called = false;
-            async Task OnNavigateAsync(NavigationContext args)
-            {
-                called = true;
-                await Task.CompletedTask;
-                throw new Exception("This is an uncaught exception.");
-            }
-            _router.OnNavigateAsync = OnNavigateAsync;
+            };
+            _router.OnNavigateAsync = new EventCallback<NavigationContext>(null, OnNavigateAsync);
 
             // Act
-            await _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateWithRefreshAsync("http://example.com/jan", false));
+            await _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateAsync("http://example.com/jan", false));
 
             // Assert
             Assert.True(called);
-            Assert.Single(_renderer.HandledExceptions);
-            var unhandledException = _renderer.HandledExceptions[0];
-            Assert.Equal("This is an uncaught exception.", unhandledException.Message);
         }
 
         [Fact]
         public async Task CanceledFailedOnNavigateAsyncDoesNothing()
         {
             // Arrange
             var onNavigateInvoked = 0;
-            async Task OnNavigateAsync(NavigationContext args)
+            Action<NavigationContext> OnNavigateAsync = async (NavigationContext args) =>
             {
                 onNavigateInvoked += 1;
                 if (args.Path.EndsWith("jan"))
                 {
                     await Task.Delay(Timeout.Infinite, args.CancellationToken);
                     throw new Exception("This is an uncaught exception.");
                 }
-            }
-            var refreshCalled = false;
+            };
+            var refreshCalled = 0;
             _renderer.OnUpdateDisplay = (renderBatch) =>
             {
-                if (!refreshCalled)
-                {
-                    refreshCalled = true;
-                    return;
-                }
-                Assert.True(false, "OnUpdateDisplay called more than once.");
+                refreshCalled += 1;
+                return;
             };
-            _router.OnNavigateAsync = OnNavigateAsync;
+            _router.OnNavigateAsync = new EventCallback<NavigationContext>(null, OnNavigateAsync);
 
             // Act
-            var janTask = _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateWithRefreshAsync("http://example.com/jan", false));
-            var febTask = _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateWithRefreshAsync("http://example.com/feb", false));
+            var janTask = _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateAsync("http://example.com/jan", false));
+            var febTask = _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateAsync("http://example.com/feb", false));
 
             await janTask;
             await febTask;
 
             // Assert that we render the second route component and don't throw an exception
             Assert.Empty(_renderer.HandledExceptions);
             Assert.Equal(2, onNavigateInvoked);
-        }
-
-        [Fact]
-        public async Task CanHandleSingleCancelledOnNavigateAsync()
-        {
-            // Arrange
-            async Task OnNavigateAsync(NavigationContext args)
-            {
-                var tcs = new TaskCompletionSource<int>();
-                tcs.TrySetCanceled();
-                await tcs.Task;
-            }
-            _renderer.OnUpdateDisplay = (renderBatch) => Assert.True(false, "OnUpdateDisplay called more than once.");
-            _router.OnNavigateAsync = OnNavigateAsync;
-
-            // Act
-            await _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateWithRefreshAsync("http://example.com/jan", false));
-
-            // Assert
-            Assert.Single(_renderer.HandledExceptions);
-            var unhandledException = _renderer.HandledExceptions[0];
-            Assert.Equal("OnNavigateAsync can only be cancelled via NavigateContext.CancellationToken.", unhandledException.Message);
+            Assert.Equal(2, refreshCalled);
         }
 
         [Fact]
         public async Task AlreadyCanceledOnNavigateAsyncDoesNothing()
         {
             // Arrange
             var triggerCancel = new TaskCompletionSource();
-            async Task OnNavigateAsync(NavigationContext args)
+            Action<NavigationContext> OnNavigateAsync = async (NavigationContext args) =>
             {
                 if (args.Path.EndsWith("jan"))
                 {
@@ -153,7 +103,7 @@ async Task OnNavigateAsync(NavigationContext args)
                     tcs.TrySetCanceled();
                     await tcs.Task;
                 }
-            }
+            };
             var refreshCalled = false;
             _renderer.OnUpdateDisplay = (renderBatch) =>
             {
@@ -164,11 +114,11 @@ async Task OnNavigateAsync(NavigationContext args)
                 }
                 Assert.True(false, "OnUpdateDisplay called more than once.");
             };
-            _router.OnNavigateAsync = OnNavigateAsync;
+            _router.OnNavigateAsync = new EventCallback<NavigationContext>(null, OnNavigateAsync);
 
             // Act (start the operations then await them)
-            var jan = _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateWithRefreshAsync("http://example.com/jan", false));
-            var feb = _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateWithRefreshAsync("http://example.com/feb", false));
+            var jan = _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateAsync("http://example.com/jan", false));
+            var feb = _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateAsync("http://example.com/feb", false));
             triggerCancel.TrySetResult();
 
             await jan;
@@ -180,16 +130,16 @@ public void CanCancelPreviousOnNavigateAsync()
         {
             // Arrange
             var cancelled = "";
-            async Task OnNavigateAsync(NavigationContext args)
+            Action<NavigationContext> OnNavigateAsync = async (NavigationContext args) =>
             {
                 await Task.CompletedTask;
                 args.CancellationToken.Register(() => cancelled = args.Path);
             };
-            _router.OnNavigateAsync = OnNavigateAsync;
+            _router.OnNavigateAsync = new EventCallback<NavigationContext>(null, OnNavigateAsync);
 
             // Act
-            _ = _router.RunOnNavigateWithRefreshAsync("jan", false);
-            _ = _router.RunOnNavigateWithRefreshAsync("feb", false);
+            _ = _router.RunOnNavigateAsync("jan", false);
+            _ = _router.RunOnNavigateAsync("feb", false);
 
             // Assert
             var expected = "jan";
@@ -200,7 +150,7 @@ async Task OnNavigateAsync(NavigationContext args)
         public async Task RefreshesOnceOnCancelledOnNavigateAsync()
         {
             // Arrange
-            async Task OnNavigateAsync(NavigationContext args)
+            Action<NavigationContext> OnNavigateAsync = async (NavigationContext args) =>
             {
                 if (args.Path.EndsWith("jan"))
                 {
@@ -217,11 +167,11 @@ async Task OnNavigateAsync(NavigationContext args)
                 }
                 Assert.True(false, "OnUpdateDisplay called more than once.");
             };
-            _router.OnNavigateAsync = OnNavigateAsync;
+            _router.OnNavigateAsync = new EventCallback<NavigationContext>(null, OnNavigateAsync);
 
             // Act
-            var jan = _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateWithRefreshAsync("http://example.com/jan", false));
-            var feb = _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateWithRefreshAsync("http://example.com/feb", false));
+            var jan = _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateAsync("http://example.com/jan", false));
+            var feb = _renderer.Dispatcher.InvokeAsync(() => _router.RunOnNavigateAsync("http://example.com/feb", false));
 
             await jan;
             await feb;
diff --git a/src/Components/test/E2ETest/Tests/RoutingTest.cs b/src/Components/test/E2ETest/Tests/RoutingTest.cs
@@ -570,14 +570,24 @@ public void OnNavigate_CanRenderUIForExceptions()
         {
             var app = Browser.MountTestComponent<TestRouterWithOnNavigate>();
 
-            // Navigating from one page to another should
-            // cancel the previous OnNavigate Task
             SetUrlViaPushState("/Other");
 
             var errorUiElem = Browser.Exists(By.Id("blazor-error-ui"), TimeSpan.FromSeconds(10));
             Assert.NotNull(errorUiElem);
         }
 
+        [Fact]
+        public void OnNavigate_CanRenderUIForSyncExceptions()
+        {
+            var app = Browser.MountTestComponent<TestRouterWithOnNavigate>();
+
+            // Should capture exception from synchronously thrown
+            SetUrlViaPushState("/WithLazyAssembly");
+
+            var errorUiElem = Browser.Exists(By.Id("blazor-error-ui"), TimeSpan.FromSeconds(10));
+            Assert.NotNull(errorUiElem);
+        }
+
         [Fact]
         public void OnNavigate_DoesNotRenderWhileOnNavigateExecuting()
         {
diff --git a/src/Components/test/testassets/BasicTestApp/RouterTest/TestRouterWithOnNavigate.razor b/src/Components/test/testassets/BasicTestApp/RouterTest/TestRouterWithOnNavigate.razor
@@ -26,9 +26,15 @@
         { "LongPage1", new Func<NavigationContext, Task>(TestLoadingPageShows) },
         { "LongPage2", new Func<NavigationContext, Task>(TestOnNavCancel) },
         { "Other", new Func<NavigationContext, Task>(TestOnNavException) },
-        {"WithParameters/name/Abc", new Func<NavigationContext, Task>(TestRefreshHandling)}
+        { "WithLazyAssembly", new Func<NavigationContext, Task>(TestOnNavException) },
+        { "WithParameters/name/Abc", new Func<NavigationContext, Task>(TestRefreshHandling) }
     };
 
+    protected override void OnAfterRender(bool firstRender)
+    {
+        Console.WriteLine("Render triggered...");
+    }
+
     private async Task OnNavigateAsync(NavigationContext args)
     {
         Console.WriteLine($"Running OnNavigate for {args.Path}...");
@@ -56,6 +62,11 @@
         throw new Exception("This is an uncaught exception.");
     }
 
+    public static Task TestOnNavSyncException(NavigationContext args)
+    {
+        throw new Exception("This is an uncaught exception.");
+    }
+
     public static async Task TestRefreshHandling(NavigationContext args)
     {
         await Task.Delay(Timeout.Infinite, args.CancellationToken);
