Skip to content
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

FakeNavigationManager registration changed from Singleton to Scoped #1408

Closed
vedion opened this issue Mar 6, 2024 · 7 comments · Fixed by #1411
Closed

FakeNavigationManager registration changed from Singleton to Scoped #1408

vedion opened this issue Mar 6, 2024 · 7 comments · Fixed by #1411

Comments

@vedion
Copy link

vedion commented Mar 6, 2024

Hi,

In this PR the registration of the FakeNavigationManager registration was changed from Singleton to Scoped:
8f99afa#diff-bf76c0726692846a886af9a9923c58a1d5e5828342c75002435ce77d764b902f

On the client the scope for the NavigationManager is Singleton:
https://learn.microsoft.com/en-us/aspnet/core/blazor/fundamentals/dependency-injection?view=aspnetcore-8.0#default-services

We have Singleton services in the client where the NavigationManager is injected but now we get this error in the bUnit tests:
"System.InvalidOperationException - Cannot consume scoped service 'Microsoft.AspNetCore.Components.NavigationManager' from singleton"

Why was this changed?

Best Regards,
Anders Havn

@linkdotnet
Copy link
Collaborator

The change was related to #1409 as well.

I don't see much of a reason at the moment not to make it Singleton again - given that it has a dependency to TestContext which is somewhat Singleton.
Any arguments speaking against that @egil ?

@egil
Copy link
Member

egil commented Mar 6, 2024

If there are no reasons to keep it at scoped. But didn't we change it to scoped due to a dotnet 8 change?

@linkdotnet
Copy link
Collaborator

as far as I remember we did this as "mitigation" for the endless recursive loop - at least all of our tests are green (including the new NavMan test)

@egil
Copy link
Member

egil commented Mar 6, 2024

Weren't there someone that raise an issue wanting this to be scoped. My brain is fried, so it could very well be it was a first attempt that was not roled back.

Let's revert and push a preview and see if somebody comes calling.

@linkdotnet
Copy link
Collaborator

linkdotnet commented Mar 6, 2024

Well - our code indicates that funny bit:

services.AddSingleton<FakeNavigationManager>();
services.AddScoped<NavigationManager>(s => s.GetRequiredService<FakeNavigationManager>());

So we can safely assume, that it doesn't matter and we can revert back :D

Will make a PR.

linkdotnet added a commit that referenced this issue Mar 6, 2024
@linkdotnet linkdotnet mentioned this issue Mar 6, 2024
linkdotnet added a commit that referenced this issue Mar 6, 2024
* fix: Make NavigationManager singleton

fixes #1408

* chore: Bump Packages
@linkdotnet
Copy link
Collaborator

linkdotnet commented Mar 6, 2024

@vedion There will be a 1.28.2-preview package on NuGet in a few minutes.
Let us know your feedback

@vedion
Copy link
Author

vedion commented Mar 7, 2024

That fixes our issue. Thank you for the fast feedback.

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 a pull request may close this issue.

3 participants