fluttercommunity / get_it

Get It - Simple direct Service Locator that allows to decouple the interface from a concrete implementation and to access the concrete implementation from everywhere in your App. Maintainer: @escamoteur
https://pub.dev/packages/get_it
MIT License
1.36k stars 149 forks source link

Get all from one type #354

Closed escamoteur closed 9 months ago

escamoteur commented 9 months ago

In the past it would throw and I argue that in 90% of cases it's a bug if you try to register more than one instance of one type. It would be a breaking change otherwise which I try to prevent. Am 20. Feb. 2024, 15:55 +0100 schrieb James H @.***>:

@hughesjs commented on this pull request. In lib/get_it.dart:

@@ -151,6 +151,11 @@ abstract class GetIt { /// If you really need to you can disable the asserts by setting[allowReassignment]= true bool allowReassignment = false;

  • /// Till V7.6.7 GetIt didn't allow to register multiple instances of the same type.
  • /// if you want to register multiple instances of the same type you can enable this
  • /// and use getAll() to retrieve all instances of that parent type
  • static bool allowRegisterMultipleImplementationsOfoneType = false; Is this wholly necessary? What was the behaviour previously? Would it error or fail silently? If it would error, I don't think this is necessary as it wouldn't break/change any already extant behaviour. If the latter, then I guess it's debatable. I don't generally think it's a good idea to have functionality dictated by flags in properties. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you modified the open/close state.Message ID: @.***>
hughesjs commented 9 months ago

In the past it would throw and I argue that in 90% of cases it's a bug if you try to register more than one instance of one type. It would be a breaking change otherwise which I try to prevent. Am 20. Feb. 2024, 15:55 +0100 schrieb James H @.>: @hughesjs commented on this pull request. In lib/get_it.dart: > @@ -151,6 +151,11 @@ abstract class GetIt { /// If you really need to you can disable the asserts by setting[allowReassignment]= true bool allowReassignment = false; + /// Till V7.6.7 GetIt didn't allow to register multiple instances of the same type. + /// if you want to register multiple instances of the same type you can enable this + /// and use getAll() to retrieve all instances of that parent type + static bool allowRegisterMultipleImplementationsOfoneType = false; Is this wholly necessary? What was the behaviour previously? Would it error or fail silently? If it would error, I don't think this is necessary as it wouldn't break/change any already extant behaviour. If the latter, then I guess it's debatable. I don't generally think it's a good idea to have functionality dictated by flags in properties. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you modified the open/close state.Message ID: @.>

I don't think this use-case is quite as esoteric as you think. That being said, maybe there's another way to handle this than a property if you do want to maintain the behaviour.

Normally, I'd suggest implementing a derived class which allows the extended behaviour here, but I don't know how well that would play with the global static approach that get it has. Maybe we stick a method on on the static class such as .enableRegisteringMultipleInstances() which substitutes the old implementation with the extended one?

It's a bit more verbose than what's here, but I think it would be easier to debug and reason with. You could also add a note to the exception pointing people towards using the new implementation if it was intentional.

hughesjs commented 9 months ago

Another issue with the current implementation, I think, would be what happens if this is set to true and later changed to false? Should it clear out multiple registrations or just prevent more? It's not clear.

escamoteur commented 9 months ago

That's a property that you only should set once. Let's keep it for the next version like this an see what feedback we receive. Am 20. Feb. 2024, 16:20 +0100 schrieb James H @.***>:

Another issue with the current implementation, I think, would be what happens if this is set to true and later changed to false? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you modified the open/close state.Message ID: @.***>

hughesjs commented 9 months ago

In that case... Can we set it through a method that fails if you try to set it more than once

escamoteur commented 9 months ago

Sure we can add an enable method Am 20. Feb. 2024, 16:25 +0100 schrieb James H @.***>:

In that case... Can we set it through a method that fails if you try to set it more than once — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you modified the open/close state.Message ID: @.***>

hughesjs commented 9 months ago

And make the prop private?

escamoteur commented 9 months ago

Yep Am 20. Feb. 2024, 16:26 +0100 schrieb James H @.***>:

And make the prop private? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you modified the open/close state.Message ID: @.***>

hughesjs commented 9 months ago

Sounds great :)