Skip to content
This repository has been archived by the owner on Jan 15, 2023. It is now read-only.

Non static IoC container. (Preparation of work for Custom IoC containers ) #106

Closed
wants to merge 1 commit into from

Conversation

Soarc
Copy link
Contributor

@Soarc Soarc commented Jun 29, 2019

Whats the point of this pull request?

Currently Chromely uses dependency injection through static IoC container. This is bad. In fact, static IoC container is not dependency injection. It is Service Location, which is anti-pattern.

This pull request tries to fix this, at least on some parts. Sure, there are many points, where it can be done better (For example in many places IChromelyContainer is still injected or used via ChromelyConfiguration), but this is just start.

I'm looking forward to hear from maintainers of this repository suggestions, how can I improve this pull request. Thanks.

What have been done:

  1. Removed all direct usages of IoC static class. All usages are replaced either with direct dependency injection, or resolved via ChromelyConfiguration.IoCContainer.
  2. Eliminated ChromelyConfiguration Static instance usages - ChromelyConfiguration now injected directly.
  3. Removed CefGlue/SharpBrowser static instance usages.
  4. Removed NativeWindow static usages.
  5. IoC, ChromelyConfiguration and ServiceRouteProvider static members are marked as obsolete.

Next steps.

  1. Refactor RequestTaskRunner to not use ServiceRouteProvider static instance.
  2. Refactor code, to not use ChromelyConfiguration.IoCContainer service location/registration. Use direct injection instead or create new services, where it is required.

@mattkol
Copy link
Member

mattkol commented Jun 30, 2019

@Soarc that is a great idea. Been thinking about this too since #91. DI was originally ignored for simplicity but I think it is time to make this a complete IOC implementation going forward, while still making it simple.

Unless there is an objection, I would suggest we @Soarc @FrankPfattheicher @Fiahblade look at this after #96 is resolved and make it part of next release.

@RupertAvery may be interested too.

Thanks.

@mattkol
Copy link
Member

mattkol commented Nov 8, 2019

@Soarc please see #135 for implementation.
Thanks.

@mattkol mattkol closed this Nov 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants