-
Notifications
You must be signed in to change notification settings - Fork 77
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
[FEATURE]-Refactor Plugin loading mechanism #1065
Comments
Every plugin needs to do the SAME thing in the |
Also, the |
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);
}
` |
Is loading the device types the responsibility of the factory then? |
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 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. |
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. |
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. |
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.
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.
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. |
Perhaps we could retitle this issue, Refactor Plugin Loading Mechanism? |
After some thought, I'm thinking that we could refactor this and move the responsibility for populating the edit to add: I think this could possibly be done as a non-breaking change, but that'll require some testing. |
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.
The text was updated successfully, but these errors were encountered: