[FEATURE]-Refactor Plugin loading mechanism

[ngenovese11]

Is your feature request related to a problem? Please describe. I am frustrated that I need to use a base class for a plugin factory, I would prefer to implement an interface. The implementation requirements of IPluginDeviceFactory are unclear. What should the LoadDeviceTypes() method do?

Describe the solution you'd like An interface (perhaps calls IEssentialsPluginFactory) with a function that accepts a DeviceConfig and return an EssentialsDevice and with a function that accepts a string and returns a bool that indicates if a device type is supported. MinimumEssentials version could be a string property.

[andrew-welker]

Every plugin needs to do the SAME thing in the LoadDeviceTypes method, which is why the IPluginDeviceFactory interface is implemented by the abstract base class EssentialPluginDeviceFactory. This is exactly the case for using an abstract base class instead of just providing an interface and depending on plugin factories to implement it correctly, as it provides common functionality that all plugin device factories need to implement. What exactly is the frustration in inheriting from an abstract base class?

[andrew-welker]

Also, the LoadDeviceTypes method for all discovered plugins is called during the startup process to populate the FactoryMethods dictionary that is used when building devices from the configuration.

[ngenovese11]

I think the below is a much cleaner implementation of a keyed factory. It is easily testable even without whatever global state is required by loaded plugins. Could it be accomplished through the current paradigm?

 public interface IPluginDeviceFactory
    {
        bool SupportsType(string deviceType);
        EssentialsDevice BuildDevice(DeviceConfig dc);
    }
    public class MockEssentialsDeviceFactory : Dictionary<string, Func<DeviceConfig, EssentialsDevice>> , IPluginDeviceFactory
    {
        public MockEssentialFactory() : base(StringComparer.OrdinalIgnoreCase)
        {
            var someOtherServiceImightNeed = new();
            Add("MockType", config => new MockDevice(config.Key, config.Name, someOtherServiceImightNeed));
        }

        public bool SupportsType(string deviceType) => ContainsKey(deviceType);

        public EssentialsDevice BuildDevice(DeviceConfig dc) => this[dc.Type](dc);
    }
`

[ngenovese11]

Also, the LoadDeviceTypes method for all discovered plugins is called during the startup process to populate the FactoryMethods dictionary that is used when building devices from the configuration.

Is loading the device types the responsibility of the factory then?

[andrew-welker]

In the most common use case, plugin cplzs are unzipped, then assemblies in the cplz are checked against currently loaded ones, with any duplicate assemblies in the plugin being ignored in preference of the assemblies included in the Essentials cpz. The unique assemblies in the cplz are then loaded using reflection. Essentials then searches through the types in the assembly to find the class(es) that implement the IPluginDeviceFactory. This Factory class is then instantiated, and the BuildDevice method is loaded in the FactoryMethods dictionary once for each type in the factory's list of types.

Some of this process is because of the limitations of loading assemblies in .NET 3.5 Compact, in which once loaded into memory, an assembly can't be unloaded, and having a single AppDomain. Some of these limitations are removed in the move to 4-series and .NET 4.7.2, but other issues arise. It might be worth it to explore changing this process and changing what plugins need to implement in order to be loaded and built.

We chose to go with an abstract base class in the interest of making it easier for plugin developers, whether PepperDashers or not, to develop plugins. An abstract base class allows for providing boilerplate code in a predictable manner that otherwise would have to be written over and over by plugin developers, leaving room open for mistakes and possibly causing reliability issues with Essentials, as the assembly loading process happens early on in the startup process of Essentials.

I'd like to understand the frustration of inheriting from a base class. To me, using the base class allows a plugin developer to focus on the needs of the plugin, without having to worry about how that plugin is consumed.

[ngenovese11]

The frustration is the base class couples me to a specific implementation when there are cleaner alternatives. I'm happy to implement LoadTypes if that is what is required, but I'd think it would be easier and clearer if that method perhaps returned something like a dictionary of keyed factory methods. The interface of IPluginDeviceFactory is not a factory contract. It doesn't even have a method that returns anything. My wish would be that the base class be available, but if someone wants custom behavior, that also be available via an interface. In the current paradigm I don't think that is the case.

[andrew-welker]

Is there a specific use case that something like this allows you to do that can't be accomplished using the current method?

I do appreciate what was posted above as a change. One issue I can see is preventing collisions between type strings in different plugins. The current plugin loading process is in Essentials Core in the plugins folder. How would that mechanism need to change to accommodate what you're proposing above?

Also, something like this would definitely be a breaking change, as interface definitions would change for sure.

[ngenovese11]

Is there a specific use case that something like this allows you to do that can't be accomplished using the current method?

No the current method works well enough. I had an idea for a cleaner solution, went to implement it, and couldn't as the current method for the most part requires you to use the base class. From there, I went down the path of trying to figure out what the base class actually does and if I could implement IEssentialsPluginFactory myself. The frustration came more at the lack of clarity of what the LoadTypes() method should do.

I do appreciate what was posted above as a change. One issue I can see is preventing collisions between type strings in different plugins. The current plugin loading process is in Essentials Core in the plugins folder. How would that mechanism need to change to accommodate what you're proposing above?

We could choose how to deal with collisions externally (iterate and throw an error if there are dups for a given type, take the first, take both, etc). The advantage is that we could switch that around without messing with the functionality of the factory itself.

Also, something like this would definitely be a breaking change, as interface definitions would change for sure.

Agree, I think we can put this in the category of a breaking refactor.

Lastly, I appreciate the difficulty in trying to provide a seamless development environment for people writing plugins. In this case our pass at it came up with some slightly smelly code. I have no doubt that we'll continue to run up against this problem and we'll have to deal with it as it comes.

[ngenovese11]

Perhaps we could retitle this issue, Refactor Plugin Loading Mechanism?

[andrew-welker]

After some thought, I'm thinking that we could refactor this and move the responsibility for populating the DeviceFactoryMethods dictionary that's used later in the process to the actual plugin loading mechanism. If the IDeviceFactory interface is changed to add the EssentialsDevice BuildDevice(DeviceConfig dc) method signature, the LoadTypes method could be removed. Then, the plugin loading mechanism, once it's located the factory and instantiated it couild iterate through the Types list and populate the directory that way. There are some details that would need to be worked out, as the current method grabs some metadata from the device class and populates some fields in the DeviceFactoryWrapper class.

edit to add: I think this could possibly be done as a non-breaking change, but that'll require some testing.