Make OnNavigateAsync EventCallback and cancel previous navigation

[captainsafia]

• Change OnNavigateAsync type to EventCallback
• Remove conditional rendering logic in RunOnNavigateAsyncWithRefresh
• Remove check for OperationCancelledException (CallStateHasChangedWhenAsyncTaskFinished ignores this)

Addresses #24215

[javiercn]

It will be good to capture the new behavior of OnUpdateDisplay, I imagine that this has changed since we are removing it.

I would suggest we capture on an integer, the number of times it is being called and check against how many times we expect it to be called. That will help catch regressions if in the future we make a change to the implementation that results in a subtle change in the rendering sequence.

[captainsafia]

Added this.

[javiercn]

Instead of removing this test, can we capture the new behavior? (Canceled OnNavigate invocations will result in a new render)

[captainsafia]

Sure.

[javiercn]

Why are we removing coverage for this? Shouldn't we still test what happens when an exception is thrown from the handler?

[captainsafia]

We have an E2E test that validates this scenario here so I removed this.

[javiercn]

That's fair, for future reference I would suggest you consider keeping the unit test and the E2E test. They complement each other.

The unit test validates the component and helps catch regressions faster while the E2E test validates the integration. Figuring out a regression with a failing E2E test is much harder than with a unit test.

[captainsafia]

Turns out that doing this in the E2E tests is the only option I have. There isn't a super nice way to create an EventCallback with the Router component instantiated with the tests (hence why we use new EventCallback(null, action). This exercises the execution of the async action but doesn't exercise the renderer.

[javiercn]

Hmm, I do know how to do this, but it requires a bit of changes. It involves:

• Creating a parent component.
• Attaching the instance to the renderer.
• Setting properties on the parent component instance.
• Triggering a render from the parent component.

There are a lot of samples in the Renderer tests, but it is a bit more involved.

[javiercn]

I'll let you use your best judgement here.

[javiercn]

Orthogonal to this change: We should test what happens when this throws synchronously and asynchronously. For example:

           Task OnNavigateAsync(NavigationContext args)
            {
                called = true;
                throw new Exception("This is an uncaught exception.");
                return Task.CompletedTask;
            }

Which throws synchronously VS the method above. (Which also throws synchronously, but the async machinery will transform it into a failed task).

For throwing asynchronously we actually need to go async

           async Task OnNavigateAsync(NavigationContext args)
            {
                called = true;
                await Task.Delay(1000); // Or better than this, a task completion source
                throw new Exception("This is an uncaught exception.");
            }

[captainsafia]

Added an E2E test for this. E2E tests work a little bit better here because there isn't a good way to attach the event callback the router component in the unit tests.

[javiercn]

Looks great, I have made a few comments about the tests, but other than that it looks good.

[javiercn]

Signing-off, but please address the test questions/changes before merging