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 Support for Keyed Registration #11

Merged
merged 6 commits into from
Apr 29, 2021
Merged

Add Support for Keyed Registration #11

merged 6 commits into from
Apr 29, 2021

Conversation

dicko2
Copy link
Contributor

@dicko2 dicko2 commented Apr 27, 2021

resolves #3

@dicko2 dicko2 changed the title add code with only singleton Add Support for Keyed Registration Apr 27, 2021
@codecov-commenter
Copy link

codecov-commenter commented Apr 27, 2021

Codecov Report

Merging #11 (54316d5) into main (9882a39) will not change coverage.
The diff coverage is 50.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #11   +/-   ##
=======================================
  Coverage   78.89%   78.89%           
=======================================
  Files          19       19           
  Lines         706      706           
=======================================
  Hits          557      557           
  Misses        149      149           
Impacted Files Coverage Δ
src/Agoda.IoC.Unity/UnityKeyedComponentResolver.cs 70.00% <ø> (ø)
src/Agoda.IoC.Core/KeyedComponentFactory.cs 73.33% <50.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9882a39...54316d5. Read the comment docs.

public class KeyTypePair
{
public Type Type { get; set; }
public object Key { get; set; }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the decision behind using Object key and not use String key? is there any use case for non-string key? so we can avoid boxing unboxing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good question, the original framework done in unit was using object as a key, so in order to make it work with the original code i needed to use object, we should probably move to string. But unsure what the impact is. I'll have a look

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated it to string, its a breaking change to the unity implementation, but i doubt anyone is using it, the unity implementation was mainly published for historical reasons.

Copy link

@joruaz joruaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had my doubt about passing the dictionary while doing the autowiring and doing the registration of the key-values afterwards. Instead of just doing the key/type registration immediately. I understand we do need to check for duplicate key/type registrations so this seems to be necessary. If there is a way where we don't need the dictionary but instead use the container registration for the same checks could be more elegant

@dicko2
Copy link
Contributor Author

dicko2 commented Apr 28, 2021

@joruaz that is how it was done in the original implementation. Because Unity the container is active while you are registering.

So you can do things like this

container.RegisterSingleton<IMyObject>(myObject);
/// later on
var tmp = container.GetService<IMyObject>();
// mutate tmp
tmp.MyList.Add(stuff);
// put tmp back in the container and override the singleton
container.RegisterSingleton<IMyObject>(tmp);

With dotnet DI it has two stages

  1. Register every
  2. Build container

You cannot call the "GetService" until its registered and you are at stage 2, this is actually better because you can do nice validation (its almost as good as simpleinjector), but it lacks some of the flexibility.

So we need someway to maintain state. Or I think another way to implement it could be to query with linq at the end might be a bit more elegant the doing it in the foreach, but I was trying to replicate code from the old implementation which is written like this, rather than thinking through too much.. I forget to think sometimes :)

Comment on lines 27 to 30
public bool IsRegistered(string key)
{
return !_registrations.TryGetValue(key, out var implementationType);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public bool IsRegistered(string key) => _registrations.TryGetValue(key, out var implementationType);

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's a ! that shouldn't be there

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch, htf did that pass the unit test is what i need to check to

@dicko2 dicko2 merged commit 3fe4b18 into main Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for keyed registration in net core
5 participants