Skip to content
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

Add Working Directory to .nxm Handler & Remove Default Host Builder (fixes #513) #555

Merged
merged 3 commits into from
Aug 14, 2023

Conversation

Sewer56
Copy link
Member

@Sewer56 Sewer56 commented Aug 14, 2023

[Change] Added Working Directory to Protocol Registration

On Linux, this involved setting the Path= field inside the .desktop file as specified in the original PR.

On Windows, it was a matter of adding an additional value named WorkingDirectory, under the same registry key as the default commandline itself.

Investigation: Folder Scanning

Original issue (#513) mentions, that the Default Host Builder is scanning the current working directory recursively.

I had a look into its code; and found out that it was searching for appconfig.json and its possible variants (e.g. appconfig.Development.json; which are part of the default MSFT configuration system.

It was searching up from what it set as the default Content Root, which defaults to current working directory in Host.CreateDefaultBuilder. It is possible to replace this after the fact before calling Build(), however...

In the case of the App, we switched from using the default config system to a custom solution a while back; the reasoning being that the default implementation does not have a means of customizing how the data is deserialized; and we needed to deserialize types that are dependent on our custom serialization system in the DataModel.

[Change] Removed Host.CreateDefaultBuilder Outright

At first I considered just fixing the content root, however I've done a quick investigation into the functionality of Host.CreateDefaultBuilder to see how much we use from it; as I was curious, especially given that we usually roll our own stuff in the app.

Action Used By App Notes
Set the ContentRootPath to the result of GetCurrentDirectory(). No We use our own Paths library for fetching content.
Load host IConfiguration from "DOTNET_" prefixed environment variables. No We roll our own config.
Load app IConfiguration from 'appsettings.json' and 'appsettings.[EnvironmentName].json'. No We roll our own config.
Load app IConfiguration from User Secrets when EnvironmentName is 'Development' using the entry assembly. No We roll our own config.
Load app IConfiguration from environment variables. No We roll our own config.
Configure the ILoggerFactory to log to the console, debug, and event source output. No We configure all logging that we actively use in-app manually.
Enable scope validation on the dependency injection container when EnvironmentName is 'Development'. No In Rider, there is no by default environment override; you need to create a custom launch profile and add it as an environment variable; so most likely none of us have actually used this before.

Given that we don't use any of its functionality, I removed the default host builder from App & Unit Tests, then verified that basic functionality works.

I had a quick walk through the code base; every hosting feature we use throughout the app; is manually configured by us at runtime, and thus we do not utilize any of the configuration the default host builder currently provides:

 - Set the ContentRootPath to the result of GetCurrentDirectory(). | We don't use ContentRoot anywhere.
- [All IConfiguration Stuff] | We roll our own configuration serialization; due to requirements of using our own JSON serialization system from the DataModel.
- Configuring ILogger | We do that manually.
- Enable Scope Validation on DI when Development Environment | We perform our own validation.
@Sewer56 Sewer56 added complexity-small Spike An investigation task meta-performance Anywhere we might get an improvement in performance. os-windows This affects Windows related code. os-linux This affects Linux related code. labels Aug 14, 2023
@Sewer56 Sewer56 added this to the v0.2 milestone Aug 14, 2023
@Sewer56 Sewer56 self-assigned this Aug 14, 2023
@Sewer56 Sewer56 requested a review from a team August 14, 2023 22:36
@halgari halgari merged commit a2e9b14 into main Aug 14, 2023
1 of 3 checks passed
@halgari halgari deleted the add-working-directory-to-nxm branch August 14, 2023 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta-performance Anywhere we might get an improvement in performance. os-linux This affects Linux related code. os-windows This affects Windows related code. Spike An investigation task
2 participants