Skip to content

Feature/navigation lock #809

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

Merged
merged 18 commits into from
Sep 15, 2022
Merged

Feature/navigation lock #809

merged 18 commits into from
Sep 15, 2022

Conversation

linkdotnet
Copy link
Collaborator

@linkdotnet linkdotnet commented Aug 1, 2022

This pull requests allows the FakeNavigationManager to prevent navigation when a NavigationLock component is existing. Fixes #804. At the moment this PR can not be merged as the feature is targeted for 7.0.0-rc.1 which will be available September. Also I targeted v2 but maybe it makes sense to have this feature available on our main branch as well. As I am mainly working on a Mac M1 I stuck to use >=net6.0. Let me know your thoughts. I am happy to target main instead of v2.

New behaviour

  • NavigationLock works with our FakeNavigationManager. That said if user code prevents navigation, so will the FakeNavigationManager
  • No new entry will be added to the history if the user prevents navigation
  • JSInterop is added to make NavigationLock work

Out of Scope

  • In bUnit this will ever work only with the NavigationManager itself. In theory the "wild" aka "real blazor" we can use the NavigationLock with stuff like that: <a href="https://github.com/internal">Text</a> and the user can prevent navigation. As we are no browser and do not have a router which would the task for us, we can't implement that. But the user can do the following in a test:
public void ShouldCancelNavigationWhenMagicCondition()
{
    var cut = RenderComponent<...>();
    var navigationManager = Services.GetRequiredService<NavigationManager>();

    // simulate the click on a href
    navigationManager.NavigateTo("/internal");

    // Do some assertion here
}

Most probable we want to have this in the documentation. Still a bit of mixed feelings about that, but I can't imagine another way to guide the user here.

@linkdotnet linkdotnet requested a review from egil August 1, 2022 13:43
@linkdotnet linkdotnet changed the base branch from main to v2 August 1, 2022 13:43
@egil
Copy link
Member

egil commented Aug 3, 2022

I'm going to get to this as soon as I get a little time to do a proper review.

@egil
Copy link
Member

egil commented Aug 4, 2022

I targeted v2 but maybe it makes sense to have this feature available on our main branch as well. As I am mainly working on a Mac M1 I stuck to use >=net6.0. Let me know your thoughts. I am happy to target main instead of v2.

Since v1 will also support .net7 I think we need to target main. I will see about getting Codespaces set up here on github such that you can work on this.

Tracking this here: dotnet-foundation/projects#218

@linkdotnet
Copy link
Collaborator Author

I targeted v2 but maybe it makes sense to have this feature available on our main branch as well. As I am mainly working on a Mac M1 I stuck to use >=net6.0. Let me know your thoughts. I am happy to target main instead of v2.

Since v1 will also support .net7 I think we need to target main. I will see about getting Codespaces set up here on github such that you can work on this.

nah don't worry. I still have my old beloved windows notebook. Everytime when I open something the fan goes crazy, but he still works ;)

That said I will retargeted the branch to main and edit the PR to target main

@egil
Copy link
Member

egil commented Aug 4, 2022

  • In bUnit this will ever work only with the NavigationManager itself. In theory the "wild" aka "real blazor" we can use the NavigationLock with stuff like that: <a href="https://github.com/internal">Text</a> and the user can prevent navigation. As we are no browser and do not have a router which would the task for us, we can't implement that. But the user can do the following in a test:

I think I need a bit more explanation on why this wont work with bUnit. Can you elaborate.

@egil
Copy link
Member

egil commented Aug 4, 2022

OK, looks good so far. I'm still vacationing, but will see if I can get back to this again later today or tomorrow with a follow-up. Thanks for your patience.

@linkdotnet
Copy link
Collaborator Author

linkdotnet commented Aug 4, 2022

  • In bUnit this will ever work only with the NavigationManager itself. In theory the "wild" aka "real blazor" we can use the NavigationLock with stuff like that: <a href="https://github.com/internal">Text</a> and the user can prevent navigation. As we are no browser and do not have a router which would the task for us, we can't implement that. But the user can do the following in a test:

I think I need a bit more explanation on why this wont work with bUnit. Can you elaborate.

Ok. Let's make it visual:

@inject NavigationManager NavigationManager
<a href="/counter">Go to Counter</a>
<button @onclick="@(() => NavigatioNmanager.NavgiateTo("/counter"))">Go to counter</button>

<NavigationLock OnBeforeInternalNavigation="PreventNavigation"></NavigationLock>

@code {
    private void PreventNavigation(LocationChangingContext context) => context.PreventNavigation();
}

Let's ignore bUnit for a second. If we have this component and open our browser, the user can click either on the button or the <a> element and will not be redirected to the counter page (thanks to the NavigationLock and OnBeforeInternalNavigation where we prevent navigation). Important is that the button uses the NavigationManager where the <a> element relies on the router and browser.

Back to bUnit. The <button> case is fairly simple:

public void ButtonCase()
{
    var cut = RenderComponent<MyComponent>();

    var button = cut.Find("button").Click();
    
    // Assert we did not go away for example via history of NavigationManager
}

But how does that work with an <a> element? Simply, it doesn't, We have no "means of clicking" on that element. There is no onclick event handler attached. We would need someone to active the <a> element (a browser) and someone who takes care of the dispatching to the navigationmanager (aka a router), both things we do not have.

The work around to test the <a> case would be:

public void ACase()
{
    var cut = RenderComponent<MyComponent>();

    // That does the same as clicking on the <a> element
    Services.GetRequiredService<NavigationManager>().NavigateTo("/counter");
    
    // Assert we did not go away for example via history of NavigationManager
}

If we are here on the same page, I can add that to the documentation, which is anyway outstanding at the moment.

@linkdotnet
Copy link
Collaborator Author

OK, looks good so far. I'm still vacationing, but will see if I can get back to this again later today or tomorrow with a follow-up. Thanks for your patience.

Then you should stay in vacationing mode ;)

Anyway we can't merge this until somewhen in September until .net7.0 rc1 hits the public

@linkdotnet linkdotnet force-pushed the feature/NavigationLock branch from 1f9bd5b to d315598 Compare August 4, 2022 14:31
@linkdotnet linkdotnet changed the base branch from v2 to main August 4, 2022 14:31
@linkdotnet
Copy link
Collaborator Author

So I rebased onto main and targeted main.

@egil
Copy link
Member

egil commented Aug 10, 2022

We could extend our NavigationHistory here as well to include whether or not a navigation attempt was completed or cancelled.

@linkdotnet
Copy link
Collaborator Author

We could extend our NavigationHistory here as well to include whether or not a navigation attempt was completed or cancelled.

Do you have a specific use case in mind? From an user point of view shouldn't it be enough to have no entry?

@egil
Copy link
Member

egil commented Aug 10, 2022

We could extend our NavigationHistory here as well to include whether or not a navigation attempt was completed or cancelled.

Do you have a specific use case in mind? From an user point of view shouldn't it be enough to have no entry?

I think there could be cases where a test wants to see if e.g. the first attempt was blocked but not the second, and that assertion might be more clear written by looking into the history and seeing the first entry being canceled and the second not. An assertion on the number of items in the history to confirm this would not necessarily be correct, since there could be other reasons why there is no entry, e.g. the CUT didn't actually attempt a navigation.

@linkdotnet
Copy link
Collaborator Author

linkdotnet commented Aug 10, 2022

We could extend our NavigationHistory here as well to include whether or not a navigation attempt was completed or cancelled.

Do you have a specific use case in mind? From an user point of view shouldn't it be enough to have no entry?

I think there could be cases where a test wants to see if e.g. the first attempt was blocked but not the second, and that assertion might be more clear written by looking into the history and seeing the first entry being canceled and the second not. An assertion on the number of items in the history to confirm this would not necessarily be correct, since there could be other reasons why there is no entry, e.g. the CUT didn't actually attempt a navigation.

I get your point but still think this will create unit tests which rely on structure or implementation details rather than actual behaviour. Your use-case describes missing unit tests which cover the other cases.

@egil
Copy link
Member

egil commented Aug 10, 2022

I get your point but still think this will create unit tests which rely on structure rather than actual behaviour. Your use-case describes missing unit tests which cover the other cases.

But with navigation from a component under test, the only place a user can verify the correct behavior is through the FakeNavigationManager, there is no other place where the effect of calling NavigateTo is observable as far as I can tell, if it's the component under test that triggers the navigation.

@linkdotnet
Copy link
Collaborator Author

But with navigation from a component under test, the only place a user can verify the correct behavior is through the FakeNavigationManager, there is no other place where the effect of calling NavigateTo is observable as far as I can tell, if it's the component under test that triggers the navigation.

That is true. My point is more what should a user test? For me: Effect / Outcome.
With your given example, yes we can have multiple situation which lead to the same outcome, but also there are two different effects leading to the outcome. So let's take your example of first attempt blocked.
You would have one test anyway which checks that the navigation was blocked. Then later you would need a setup (aka effect) which gives the expected navigation (aka non-blocking).

Both cases are testable with the current approach without any boilerplate code. I hope you can understand my motivation behind my questioning.

@egil
Copy link
Member

egil commented Aug 10, 2022

I hope you can understand my motivation behind my questioning.

I think I get where you are coming from. I just have feeling that there will be users who will have a weird need to write that assertion, and will request that we add that at some point, maybe they have something crazy like a timer based navigation. It would be a breaking change adding it later, so that's also a motivation for including it now.

@linkdotnet
Copy link
Collaborator Author

I hope you can understand my motivation behind my questioning.

I think I get where you are coming from. I just have feeling that there will be users who will have a weird need to write that assertion, and will request that we add that at some point, maybe they have something crazy like a timer based navigation. It would be a breaking change adding it later, so that's also a motivation for including it now.

Then let's do it. From a design point of view I guess we will always push an entry on the NavigationHistory stack but we will introduce a new boolean flag indicating if the navigation would have been cancelled. Does that sounds reasonable to you?

@egil
Copy link
Member

egil commented Aug 10, 2022

Then let's do it. From a design point of view I guess we will always push an entry on the NavigationHistory stack but we will introduce a new boolean flag indicating if the navigation would have been cancelled. Does that sounds reasonable to you?

I think we also need to actually cancel the navigation like you do in your PR here, so yeah, the flag should be something like NavigationCancelled or perhaps NavigationCompleted, if we want to be more "positive" :). For naming, I think it will be more discoverable to users if it includes the word "cancelled", as that's the feature name in Blazor.

@linkdotnet
Copy link
Collaborator Author

What is public string? State { get; } for?

Ahh forgot that NavigationOptions Options { get; } has the state. It seems redundant to add it as a property directly on NavigationHistory as well and just point it to Options.HistoryEntryState?

Yes exactly. We could do public string? State => Options.HistoryEntryState but I am not sure what we gain from that.

@egil
Copy link
Member

egil commented Aug 11, 2022

What is public string? State { get; } for?

Ahh forgot that NavigationOptions Options { get; } has the state. It seems redundant to add it as a property directly on NavigationHistory as well and just point it to Options.HistoryEntryState?

Yes exactly. We could do public string? State => Options.HistoryEntryState but I am not sure what we gain from that.

You could argue the gain is that State and Exception are available at the same "level", next to Status, which would be where I would expect it to be if it was also not attached to Options`.

But that also seem a bit overdone, perhaps lets leave it out for now and see if users are confused.

@linkdotnet
Copy link
Collaborator Author

What is public string? State { get; } for?

Ahh forgot that NavigationOptions Options { get; } has the state. It seems redundant to add it as a property directly on NavigationHistory as well and just point it to Options.HistoryEntryState?

Yes exactly. We could do public string? State => Options.HistoryEntryState but I am not sure what we gain from that.

You could argue the gain is that State and Exception are available at the same "level", next to Status, which would be where I would expect it to be if it was also not attached to Options`.

But that also seem a bit overdone, perhaps lets leave it out for now and see if users are confused.

Yes. I like that. If we add it preemptively without need we also have to maintain that in case of changes.

@linkdotnet
Copy link
Collaborator Author

I added changelog as well as docu

@egil egil marked this pull request as draft August 15, 2022 11:38
@egil
Copy link
Member

egil commented Aug 15, 2022

Converting it to draft as it is not runnable and thus mergable yet.

@linkdotnet
Copy link
Collaborator Author

.net 7.0-rc1 is released so this PR can go live (in theory).

@linkdotnet linkdotnet marked this pull request as ready for review September 15, 2022 08:55
Copy link
Member

@egil egil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC this is already reviewed and looks great.

@linkdotnet
Copy link
Collaborator Author

You wanna wait until we fixed the WaitFor issue to reduce noise or you don’t mind?

@egil
Copy link
Member

egil commented Sep 15, 2022

This should not make a difference, so this can be merged.

@egil egil merged commit 2931a45 into main Sep 15, 2022
@egil egil deleted the feature/NavigationLock branch September 15, 2022 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow cancellation of navigation with bUnits NavigationManager
2 participants