-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Allow cancellation of navigation events in Blazor #42638
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
Changes from 20 commits
a7ae3e9
05dc74b
c1f8602
0af6785
298117a
08bc4f0
f8bfc12
ff6b245
1a7aca5
7086ddd
7edfc97
055bedf
0cea6cd
804b451
01c6484
ab6caad
bb4d33b
6f449bb
2fca4ce
3365322
54d7bca
72230e2
4e3cca2
9bfc11e
7684aeb
8e2b8c0
cf17ac7
2771923
22997be
8c017b8
61b99ed
d155810
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
namespace Microsoft.AspNetCore.Components.Routing; | ||
|
||
/// <summary> | ||
/// Contains context for a change to the browser's current location. | ||
/// </summary> | ||
public class LocationChangingContext | ||
{ | ||
private readonly CancellationTokenSource _cts; | ||
|
||
internal LocationChangingContext(string destinationUrl, bool isNavigationIntercepted, CancellationTokenSource cts) | ||
MackinnonBuck marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Marking this Since everything else about this class is public, could we give it a public constructor too? Unless you're anticipating a particular problem we'd be getting into. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the ping Steve. I don't think its a problem in this case, as it looks as if That said, I'm checking this from my phone while on vacation, so cannot be 100% sure. What do you think @linkdotnet? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From the usages of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @egil Anyway there is stuff we have to do once the PR is merged. Our custom |
||
{ | ||
TargetLocation = destinationUrl; | ||
IsNavigationIntercepted = isNavigationIntercepted; | ||
|
||
_cts = cts; | ||
} | ||
|
||
/// <summary> | ||
/// Gets the target location. | ||
/// </summary> | ||
public string TargetLocation { get; } | ||
|
||
/// <summary> | ||
/// Gets whether this navigation was intercepted from a link. | ||
/// </summary> | ||
public bool IsNavigationIntercepted { get; } | ||
|
||
/// <summary> | ||
/// Gets a <see cref="System.Threading.CancellationToken"/> that can be used to determine if this navigation was canceled. | ||
MackinnonBuck marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// </summary> | ||
public CancellationToken CancellationToken => _cts.Token; | ||
|
||
/// <summary> | ||
/// Prevents this navigation from continuing. | ||
/// </summary> | ||
public void PreventNavigation() | ||
{ | ||
_cts.Cancel(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -536,6 +536,31 @@ await Renderer.Dispatcher.InvokeAsync(() => | |
} | ||
} | ||
|
||
public async Task OnLocationChangingAsync(int callId, string uri, bool intercepted) | ||
{ | ||
AssertInitialized(); | ||
AssertNotDisposed(); | ||
|
||
try | ||
{ | ||
var shouldContinueNavigation = await Renderer.Dispatcher.InvokeAsync(async () => | ||
{ | ||
Log.LocationChanging(_logger, uri, CircuitId); | ||
var navigationManager = (RemoteNavigationManager)Services.GetRequiredService<NavigationManager>(); | ||
return await navigationManager.HandleLocationChangingAsync(uri, intercepted); | ||
}); | ||
|
||
await Client.SendAsync("JS.EndLocationChanging", callId, shouldContinueNavigation); | ||
} | ||
catch (Exception ex) | ||
{ | ||
// Exceptions thrown by location changing handlers should be treated like unhandled exceptions in user-code. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we should really be treating exceptions thrown in location changing handlers as non-fatal? In the current WebAssembly implementation of this feature, the navigation is canceled if an exception is thrown in the location changing handler. Should we treat it as a critical failure there instead? And, if we decide to make this case non-fatal, should we let the navigation continue if no other handlers cancel it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd err on the side of treating unhandled exceptions as fatal unless there's a definite use case in which developers can't be expected to avoid unhandled exceptions. In this case I expect a developer could use try/catch inside their handler if they are doing something that might fail. Generally in WebAssembly we've had a more laissez-faire attitude because we're inheriting the JS semantics around errors being something you log but may be able to continue afterwards. On Server, it's much more important to have clearly defined rules about when you can and can't continue after an exception.
If possible, that would be nice. But as long as something ends up in the JS console giving the exception details, that's enough. It just has to be reasonable for the developer to see what went wrong. |
||
Log.LocationChangeFailedInCircuit(_logger, uri, CircuitId, ex); | ||
await TryNotifyClientErrorAsync(Client, GetClientErrorMessage(ex, $"Location changing to '{uri}' failed.")); | ||
UnhandledException?.Invoke(this, new UnhandledExceptionEventArgs(ex, isTerminating: false)); | ||
} | ||
} | ||
|
||
public void SetCircuitUser(ClaimsPrincipal user) | ||
{ | ||
// This can be called before the circuit is initialized. | ||
|
@@ -730,6 +755,9 @@ public static void CircuitHandlerFailed(ILogger logger, CircuitHandler handler, | |
[LoggerMessage(210, LogLevel.Debug, "Location change to '{URI}' in circuit '{CircuitId}' failed.", EventName = "LocationChangeFailed")] | ||
public static partial void LocationChangeFailed(ILogger logger, string uri, CircuitId circuitId, Exception exception); | ||
|
||
[LoggerMessage(211, LogLevel.Debug, "Location is about to change to {URI} in ciruit '{CircuitId}'.", EventName = "LocationChanging")] | ||
public static partial void LocationChanging(ILogger logger, string uri, CircuitId circuitId); | ||
|
||
[LoggerMessage(212, LogLevel.Debug, "Failed to complete render batch '{RenderId}' in circuit host '{CircuitId}'.", EventName = "OnRenderCompletedFailed")] | ||
public static partial void OnRenderCompletedFailed(ILogger logger, long renderId, CircuitId circuitId, Exception e); | ||
|
||
|
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Uh oh!
There was an error while loading. Please reload this page.