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

[API Proposal]: Ioc can load configuration from config file #71832

Closed
soroshsabz opened this issue Jul 8, 2022 · 12 comments
Closed

[API Proposal]: Ioc can load configuration from config file #71832

soroshsabz opened this issue Jul 8, 2022 · 12 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-DependencyInjection

Comments

@soroshsabz
Copy link
Contributor

soroshsabz commented Jul 8, 2022

Background and motivation

ITNOA

I think it is very useful to add ability to load configuration from file in Ioc, Some of values of this features

  1. We can change configuration without needing recompile application.
  2. In this scenario we can create multiple config set in multiple files and load them in specific situation without needing to recompile application.
  3. We can change app behavior in runtime (or in deployment) without needing to reinstall app.

API Proposal

namespace Microsoft.Extensions.DependencyInjection
{
    /// <summary>
    /// Extension methods for adding services to an <see cref="IServiceCollection" />.
    /// </summary>
    public static class ServiceCollectionServiceExtensions
    {
         public static IServiceCollection AddFromConfig(this IServiceCollection services, IConfigurationSection config) {}
    }

API Usage

Ioc.Default.ConfigureServices(
      new ServiceCollection()
      .AddFromConfig(config.GetSection("Ioc"))
      .BuildServiceProvider());

Alternative Designs

            // Register services
            Ioc.Default.ConfigureServices(
                new ServiceCollection()
                .AddSingleton<IDialogService, DialogService>() //Services
                .AddSingleton<IFilesService, FilesService>()
                .AddSingleton<ISettingsService, SettingsService>()
                .AddSingleton(RestService.For<IRedditService>("https://www.reddit.com/"))
                .AddSingleton(RestService.For<IContactsService>("https://randomuser.me/"))
                .AddTransient<PostWidgetViewModel>() //ViewModels
                .AddTransient<SubredditWidgetViewModel>()
                .AddTransient<ContactsListWidgetViewModel>()
                .AddTransient<AsyncRelayCommandPageViewModel>()
                .AddTransient<IocPageViewModel>()
                .AddTransient<MessengerPageViewModel>()
                .AddTransient<ObservableObjectPageViewModel>()
                .AddTransient<ObservableValidatorPageViewModel>()
                .AddTransient<ValidationFormWidgetViewModel>()
                .AddTransient<RelayCommandPageViewModel>()
                .AddTransient<CollectionsPageViewModel>()
                .AddTransient<SamplePageViewModel>()
                .BuildServiceProvider());
        }

Risks

No response

@soroshsabz soroshsabz added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jul 8, 2022
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 8, 2022
@soroshsabz
Copy link
Contributor Author

related to CommunityToolkit/dotnet#328

@soroshsabz
Copy link
Contributor Author

@dotnet/area-extensions-dependencyinjection

@soroshsabz
Copy link
Contributor Author

@ericstj

@Sergio0694
Copy link
Contributor

Sergio0694 commented Jul 8, 2022

Without getting to the merit of this proposal per se, I would remove that Ioc.Default.ConfigureServices call from it since that's unrelated and might just be confusing, as the API described would simply be an extension for IServiceCollection 🙂

@ghost
Copy link

ghost commented Jul 8, 2022

Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

ITNOA

I think it is very useful to add ability to load configuration from file in Ioc, Some of values of this features

  1. We can change configuration without needing recompile application.
  2. In this scenario we can create multiple config set in multiple files and load them in specific situation without needing to recompile application.
  3. We can change app behavior in runtime (or in deployment) without needing to reinstall app.

API Proposal

namespace Microsoft.Extensions.DependencyInjection
{
    /// <summary>
    /// Extension methods for adding services to an <see cref="IServiceCollection" />.
    /// </summary>
    public static class ServiceCollectionServiceExtensions
    {
         public static IServiceCollection AddFromConfig(this IServiceCollection services, IConfigurationSection config) {}
    }

API Usage

Ioc.Default.ConfigureServices(
      new ServiceCollection()
      .AddFromConfig(config.GetSection("Ioc"))
      .BuildServiceProvider());

Alternative Designs

            // Register services
            Ioc.Default.ConfigureServices(
                new ServiceCollection()
                .AddSingleton<IDialogService, DialogService>() //Services
                .AddSingleton<IFilesService, FilesService>()
                .AddSingleton<ISettingsService, SettingsService>()
                .AddSingleton(RestService.For<IRedditService>("https://www.reddit.com/"))
                .AddSingleton(RestService.For<IContactsService>("https://randomuser.me/"))
                .AddTransient<PostWidgetViewModel>() //ViewModels
                .AddTransient<SubredditWidgetViewModel>()
                .AddTransient<ContactsListWidgetViewModel>()
                .AddTransient<AsyncRelayCommandPageViewModel>()
                .AddTransient<IocPageViewModel>()
                .AddTransient<MessengerPageViewModel>()
                .AddTransient<ObservableObjectPageViewModel>()
                .AddTransient<ObservableValidatorPageViewModel>()
                .AddTransient<ValidationFormWidgetViewModel>()
                .AddTransient<RelayCommandPageViewModel>()
                .AddTransient<CollectionsPageViewModel>()
                .AddTransient<SamplePageViewModel>()
                .BuildServiceProvider());
        }

Risks

No response

Author: soroshsabz
Assignees: -
Labels:

api-suggestion, untriaged, area-Extensions-DependencyInjection

Milestone: -

@eerhardt
Copy link
Member

eerhardt commented Jul 8, 2022

I don't believe this is a route we want to take in the core libraries. It completely removes any trimming / AOT / etc. capabilities in the application. Also, the information in the configuration is strings, which then the library would need to do the assembly loading and Type resolving from strings. This would break if the assemblies aren't referenced and in the .deps.json file.

This sounds like a great extension someone can build outside of the core Microsoft.Extensions libraries, if it is needed. It doesn't need to be in the core libraries.

We can change app behavior in runtime (or in deployment) without needing to reinstall app.

The DI container is static for the lifetime of the app. You can't make a change to the file, and then expect the running app to rebuild the ServiceProvider with the new services. The process needs to terminate and restart in order for this to happen.

cc @davidfowl

@davidfowl
Copy link
Member

Agree this should be an external library, not built in.

@soroshsabz
Copy link
Contributor Author

@Sergio0694 As you can see @davidfowl says this extension must be external of runtime, so I think it is go to the CommunityToolkit, Did you agree with me?

@soroshsabz
Copy link
Contributor Author

The DI container is static for the lifetime of the app. You can't make a change to the file, and then expect the running app to rebuild the ServiceProvider with the new services. The process needs to terminate and restart in order for this to happen.

@eerhardt I say with this feature we does not need to recompile it, as you says it is require to restart app, and I agree with you, but rebuilding is very worst than re-running ;)

@Sergio0694
Copy link
Contributor

@soroshsabz What David said is absolutely valid, but that doesn't mean this belongs in the MVVM Toolkit — it simply doesn't belong in either project. I share the concerns that Eric raised with respect to trimming etc., but most importantly, as I've already said — the MVVM Toolkit doesn't have a dependency on Microsoft.Extensions.DependencyInjection, so there's no way this extension could live there anyway even if we wanted to. And taking that dependency would be out of the question, as it would add unnecessary binary size to consuming projects that are using other service providers. For instance, it would be an immediate game breaker for us in the Microsoft Store. If you wanted that extension for yourself, you should just define it in your own project, or create your own library to host it if you wanted to share it with others too 🙂

@davidfowl
Copy link
Member

Maybe https://github.com/khellang/Scrutor is a reasonable place. Try asking on that repo if the author is willing. Otherwise just make a new package and put it on nuget.

@soroshsabz soroshsabz closed this as not planned Won't fix, can't repro, duplicate, stale Jul 10, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jul 10, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-DependencyInjection
Projects
None yet
Development

No branches or pull requests

5 participants