Let's say I have this service abstraction exposed from a library.
public interface INavigator
{
ImmutableList<IPageViewModel> Entries { get; }
void NavigateForward(IPageViewModel page);
void RemoveEntries(int indices);
void NavigateBack();
}
The library also exposes a default implementation that takes in an ILogger
as dependency.
Each method uses the logger.
public class DefaultNavigator : INavigator
{
private ILogger _logger;
public DefaultNavigator(ILogger logger)
{
_logger = logger;
}
public void NavigateForward(IPageViewModel page)
{
// [...]
_logger.LogInformation("Navigated to {ViewModelName}.", page.Name);
}
// [...]
}
The library also exposes extensions methods on INavigator
to add functionality, in the same fashion as System.Linq
(where the IEnumerable<T>
interface is very simple and all the core logic comes from the extension methods).
In our case, it makes sense to implement some logs in those extension methods.
public static class NavigatorExtensions
{
public static void ReturnToRoot(INavigator navigator)
{
var logger = ???
// [...]
logger.LogInformation("Returned to root of navigation stack.");
}
}
Here's the question: Where should we get the ILogger
from?
What we currently have
We currently can access the IServiceProvider
statically, which allows us to get an ILogger
from anywhere. However, we want to move away from that public static
pattern because we want to have multiple IServiceProvider
instances in the same process.
Options I'm considering
1. Expose ILogger
from INavigator
Does it make sense though?
public interface INavigator
{
ImmutableList<IPageViewModel> Entries { get; }
void NavigateForward(IPageViewModel page);
void RemoveEntries(int indices);
void NavigateBack();
// This isn't related to navigation.
ILogger Logger { get; }
}
- It doesn't feel quite right because that new member doesn't have anything to do with navigation.
2. Create a new interface that exposes an ILogger and soft cast in the extension method
// Not the best name, but I'm trying to avoid conflicts with the existing ILoggerProvider.
public interface ILoggerSource
{
ILogger Logger { get; }
}
The extension method would do something like this:
public static class NavigatorExtensions
{
public static void ReturnToRoot(INavigator navigator)
{
var logger = (navigator as ILoggerSource)?.Logger ?? NullLogger.Instance;
// [...]
logger.LogInformation("Returned to root of navigation stack.");
}
}
- If this was the proper solution, I think
ILoggerSource
would already exist in MS libraries. May be it does, but I didn't see it? - New implementations of
INavigator
don't have any clue that logs from extensions methods will not to work unless they also implementILoggerSource
.
3. I'm open to other options :)
Conclusion
As of writing this, I think option 1 is currently the lesser evil because it doesn't hide functionality, but my engineer mind is not satisfied.
4 Answers 4
The problem isn't just logging-- it's any time you want to access a service from an extension method. If you're going to use dependencies in your methods you should give them an instance.
If you want to have helper methods that do complicated stuff, then use a proper helper class, and accept the INavigator
as an argument.
class NavigatorHelper
{
protected readonly ILogger _logger;
protected readonly IOtherDependency _otherDependency;
public NavigatorHelper(Ilogger logger, IOtherDependency otherDependency)
{
_logger = logger;
_otherDependency = otherDependency;
}
public void ReturnToRoot(INavigator navigator)
{
_logger.LogInformation("Returned to root of navigation stack.");
}
}
//Usage
helper.ReturnToRoot(someInstance);
I get that you want to be like LINQ but LINQ's methods are all idempotent and don't mutate anything, and they only depend on what you explicitly pass them.
New option: Use a static ConditionalWeakTable
I didn't think of this earlier but another thing that could work is use a static ConditionalWeakTable to create pairs of INavigator
and ILogger
.
Something like this (null scenarios could be improved, but it's just to show how it could look).
public static class NavigatorExtensions
{
private static readonly ConditionalWeakTable<INavigator , ILogger> _loggers = new ConditionalWeakTable<INavigator , ILogger>();
public static void SetLogger(this INavigator navigator, ILogger logger)
{
_loggers.Add(navigator, logger);
}
public static ILogger GetLogger(this INavigator navigator)
{
if (_loggers.TryGetValue(navigator, out var logger))
{
return logger;
}
return null;
}
public static void ReturnToRoot(INavigator navigator)
{
var logger = navigator.GetLogger() ?? NullLogger.Instance;
// [...]
logger.LogInformation("Returned to root of navigation stack.");
}
}
Then from the startup, the INavigator
would need to set its ILogger
explicitly.
// [...]
var navigator = serviceProvider.GetRequiredService<INavigator>();
var logger = serviceProvider.GetRequiredService<ILogger<INavigator>>();
navigator.SetLogger(logger);
That doesn't seem too bad actually.
- Adding new extension methods can leverage the
GetLogger
method. - Changes are restricted to library code and app configuration. (No changes are required in existing app library-usage code.)
Option 3. don't use extension methods.
public class NavigatorExtension
{
public NavigatorExtension(ILogger logger) {...}
public void ReturnToRoot(INavigator navigator)
{
navigator.. whatever
logger.LogInformation("Returned to root of navigation stack.");
}
}
OR
public class Navigator2 : DefaultNavigator
{
public void ReturnToRoot()
{
this.Whatever...
logger.LogInformation("Returned to root of navigation stack.");
}
}
-
I'm not convinced by your second suggestion because I want ReturnToRoot to be available for INavigator (not just DefaultNavigator). Your first suggestion works, but complexifies the application code a little: Consumers would need to [resolve/receive from injection] both INavigator AND NavigatorExtension in order to use ReturnToRoot.Batesias– Batesias2022年04月10日 15:48:57 +00:00Commented Apr 10, 2022 at 15:48
-
if you think about it, those are also flaws in extension methodsEwan– Ewan2022年04月10日 16:54:45 +00:00Commented Apr 10, 2022 at 16:54
A possible alternative could be to build a custom extended INavigator
which includes those extension methods as ordinary methods, then extend the DefaultNavigator
to use the new interface, passing the logger in:
public interface IExtendedNavigator : INavigator
{
void ReturnToRoot();
// TODO - other replacements for extension methods here
}
public class ExtendedDefaultNavigator : DefaultNavigator, IExtendedNavigator
{
private readonly ILogger _logger;
public ExtendedDefaultNavigator(ILogger logger) : base(logger)
{
_logger = logger;
}
public void ReturnToRoot()
{
NavigatorExtensions.ReturnToRoot(this);
_logger.LogInformation("Returned to root of navigation stack.");
}
}
This would allow any usages in your program of INavigator
to be substituted by IExtendedNavigator
, then replacing DefaultNavigator
with ExtendedDefaultNavigator
- for example:
var logger = serviceProvider.GetService<ILoggerFactory>()
.CreateLogger(nameof(Program));
IExtendedNavigator navigator = new ExtendedDefaultNavigator(logger);
// Calls ExtendedDefaultNavigator.ReturnToRoot() instead of the extension method.
navigator.ReturnToRoot();
A word of caution - there is a potential 'gotcha' here in that code attempting to consume the ExtendedDefaultNavigator
should avoid using the INavigator
interface because that would fall back on the extension methods:
INavigator navigator = new ExtendedDefaultNavigator(logger);
// Warning - calls the extension method without logging!
navigator.ReturnToRoot();
-
I understand the suggestion, but I think the "gotcha" you pointed could be problematic (especially error-prone). Also, it seems like the solution doesn't scale well with additional extension methods (either from within the library or from the consuming application code) because they would require changes to the IExtendedNavigator OR involve additional interfaces. One of my goal is to keep the original interface as simple as possible to easily allow new implementations.Batesias– Batesias2022年04月09日 23:14:46 +00:00Commented Apr 9, 2022 at 23:14
Explore related questions
See similar questions with these tags.
INavigator
could be more explicit by using C# 8 nullables and having theILogger
be nullable (or not) so that extension methods would know whether to null check or assume a non-null value. That way implementers could use eithernull
orNullLogger
depending on the definition.