Part 2 in the move to IOCing the world. #199
Conversation
|
|
||
| private static void RegisterNetDaemonAssembly(IServiceCollection services) | ||
| { | ||
| if (BypassLocalAssemblyLoading()) |
There was a problem hiding this comment.
Do I have this arse about?
There was a problem hiding this comment.
Not sure what you asking ?
There was a problem hiding this comment.
I'm wondering if it would be better to inject both compilers and pick the right one at runtime (depending on the ENV var) in a factory. But this is something that could be easily added in the future, it was just a thought I had.
There was a problem hiding this comment.
Ohh right, interesting idea @BeeHiveJava
| private static void RegisterNetDaemonAssembly(IServiceCollection services) | ||
| { | ||
| if (BypassLocalAssemblyLoading()) | ||
| services.AddSingleton<IDaemonAppCompiler, LocalDaemonAppCompiler>(); |
There was a problem hiding this comment.
Split up local app assembly loading vs compiling .cs files from apps folder.
| _netDaemonSettings = netDaemonSettings; | ||
| } | ||
|
|
||
| public IEnumerable<Type> GetApps() |
There was a problem hiding this comment.
I created this to help with migration. Allows me to change less code.
I would like to instantiate apps using DI.
A POC I created worked but I didn't get round to how the apps would react when a re-connection would occur, so see what happens.
There was a problem hiding this comment.
I test this manually during development. And yea would be nice to create apps with DI but... still needs to be dynamic. I do not want the user to specify explicit what apps should be compiled.
There was a problem hiding this comment.
I'm angling for the app to register all the apps in the DI container. POC didn't require this, it just did reflection over app assembly and registered them in the DI container, but as I'm implementing changes things are turning out slightly different than I expected.
| return (assembly.GetTypesWhereSubclassOf<NetDaemonAppBase>(), string.Empty); | ||
| } | ||
|
|
||
| public static Assembly GetCompiledAppAssembly(out CollectibleAssemblyLoadContext alc, string codeFolder, ILogger logger) |
There was a problem hiding this comment.
I expect all this to be eventually moved into NetDaemon.Service.App.DaemonAppCompiler.
Future feature, recompile app folder and reload without having to restart docker container?
There was a problem hiding this comment.
This once worked but was moved to be compiled by container cause I wanted to support custom csproj file. We need to be able to support this in that case. Would be great if we manage this.
| { | ||
| loadedDaemonApps = null; | ||
| var alcWeakRef = new WeakReference(alc, trackResurrection: true); | ||
| alc.Unload(); |
There was a problem hiding this comment.
alc was already being unloaded right after apps were being loaded in GetCompiledAppAssembly(out CollectibleAssemblyLoadContext alc, string codeFolder, ILogger logger)
This caused an exception on app shutdown.
|
|
||
| private static bool BypassLocalAssemblyLoading() | ||
| { | ||
| var value = Environment.GetEnvironmentVariable("HASS_DISABLE_LOCAL_ASM"); |
There was a problem hiding this comment.
So this should always be added to the docker file then? When using default container/add-on? Maybe this should be added to normal config?
src/DaemonRunner/DaemonRunner/Infrastructure/Extensions/AssemblyExtensions.cs
Outdated
Show resolved
Hide resolved
Agree nice |
|
@asherw is your plan to update the PR? Or do you want to merge it as is? |
* Part 2 in the move to IOCing the world. * Remove unused code and reduce if statement nesting. * fix: ensure types are instantiable * refactor: use bool parse as return value Co-authored-by: Lesley Vente <BeeHiveJava@users.noreply.github.com>
Breaking change
Appears to load the 2 hello world apps...
Proposed change
Type of change
Additional information
The goal is to move away from Activator.CreateInstance and to pull these from IOC.
Checklist
If user exposed functionality or configuration variables are added/changed: