Redesign hooks to support DI

[tidyui]

The current hooks need to be redesigned to support dependency injection. This will break backwards compatibility for existing hook implementations.

[jensbrak]

I realize how useful this one would be. As I play around with hooks it is obvious that while they are really useful they are limited by not supporting DI. It would be logical to be able to inject a service that handles the different functionality you'd want to hook, like comment moderation and so on.

Regarding breaking backward compatibility, how common is it to use hooks "out there"? Seems to me the way they're designed they can't be the backbone of a site but maybe I'm wrong?

[r4g-jon]

We're using hooks on our project.

We use the sitemap hook to add in some non-Piranha content and we also use on the one when you save the page to do some validation checks and also write some content to another database. Being able to use dependency injection would be a really big help for us.

[tidyui]

Changing the implementation will break all existing hook implementations but all hooks will be migrated in the same way and there will be a simple document to follow on how to upgrade them.

[jensbrak]

Changing the implementation will break all existing hook implementations but all hooks will be migrated in the same way and there will be a simple document to follow on how to upgrade them.

Yes, it seems rather straight forward to use DI for hooks, but I'd not try to do a PR on the implementation of DI support for the hooks. Not yet. ;) (But I do have an idea about how it would look like on a general level)

[tidyui]

Well the hooks are currently implemented using delegates which can't support DI since they have a fixed set of parameters. This means the hooks can't be defined with lambdas as there are no fixed delegate signature to implement. What I think I sketched on before when I wrote the issue was the following. Consider we have the following method in the hook manager:

HookManager

public void RegisterOnLoad<T, THooks>(string methodName = null)
{
    if (methodName == null)
    {
        methodName = $"{ typeof(T).Name }OnLoad";
    }
    var method = typeof(THooks).GetMethod(methodName, BindingFlags.IgnoreCase | BindingFlags.Public | BindingFlags.Static);

    if (method != null)
    {
        _onLoad[typeof(T)] = method;
    }
}

This method takes the model type and the type implementing the hook as type arguments. If no method name is provided it calculates it from model type and event type. In the case of comments the name would be CommentOnLoad.

When calling the hook this could for example be implemented like this in the HookManager.

public void OnLoad<T>(T model)
{
    if (_onLoad.TryGetValue(typeof(T), out var method))
    {
        var parameters = new List<object>();

        foreach (var param in method.GetParameters())
        {
            if (param.ParameterType == typeof(T))
            {
                // Resolve using given parameter
                parameters.Add(model);
            }
            else
            {
                // Resolve using service provider
            }
        }
        method.Invoke(null, parameters.ToArray());
    }
}

This would add the provided model as parameter and resolve the other parameters from the current service collection.

Usage

The most important part here is of course usage. The difference from lambdas would be that you would need at least one class, but you could have different classes for different model types for structure. In this simplified example I'll call the class MyHooks as it will just contain a single hook.

public class MyHooks
{
    public static void CommentOnLoad(IApi api, Comment comment, IExternalDb db)
    {
        // Do cool stuff here
    }
}

This could be registered in Startup.cs using the following line:

App.Hooks.RegisterOnLoad<Comment, MyHooks>();

This is just what I had in mind back when I opened the issue. If anyone has any other ideas for how this could be implemented don't hesitate to let me know!

Regards

[jensbrak]

I see no problem with losing lambdas in favor of DI. The suggestion seems logical and rather neat I think.
OnLoad is just an example and there would be more event types I presume?

(What I had in mind was an approach inspired by how Windows Forms works with delegates for specified events like Load/Close/KeyPress etc. There's nothing preventing classes inheriting Winds Forms components to use DI. But maybe such an approach might be too complex and way too flexible to be suitable here. I think the proposed solution is more spot on and fit for purpose)

[CrossBound]

I think using an interface would be a good step in solving this. For example, if you have an IHook interface

public interface IHook
{
	async Task Execute(CancellationToken CancelToken);
}

then you can dynamically have your service provider query the assembly for all instances of this interface and populate their constructors directly from the DI provider (e.g. https://autofaccn.readthedocs.io/en/latest/register/scanning.html ). I used this to solve a task-based worker service and it allows me to implement any number of tasks by simply adding a new instance of a class that inherits from my interface.

In other words... the class constructor would define the injected services, and you simply call the Execute method on the instance when triggering the hook. Inside the Execute method the class can use any number of the services that it received during construction.

You could have different interfaces for different hook types. By using an interface you can avoid the requirement of using reflection to execute the hook. You could even have different parameters for the execute method based on what type of hook it is... for example if you are using a comment hook you might pass the comment into the execute method.

If you are using Pure DI and are not using a DI provider library, you can still use the interface approach to avoid reflection when executing the hook but you would have to use reflection to pass parameters into the constructor. I'm guessing you could also cache the results of the reflection so that you only have to take the performance hit of using reflection once during startup.

[tidyui]

@CrossBound Absolutely, the solution I proposed is all about being simple and quick to implement for the developer, this is why we've used delegates so you can quickly declare hooks with lambdas. Like you say, the reflection parts can easily be cached when adding a hook, the code I posted above was a simple implementation to show how it would be used in the client application.

[tidyui]

@CrossBound If we're considering performance, building it around interfaces and passing in parameters through the constructor means that all hooks will either have to have a scoped lifetime or be created each time they're called as most service dependencies are scoped. If hooks were to be created each time they're called this means allocating extra objects each api-call. The solution I proposed will instead inject parameters on method call, more like DI is implemented for middleware in ASP.NET.