Get all from one type #354
Conversation
| /// 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; |
There was a problem hiding this comment.
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.
|
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 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. |
|
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. |
|
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: ***@***.***>
|
|
In that case... Can we set it through a method that fails if you try to set it more than once |
|
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: ***@***.***>
|
|
And make the prop private? |
|
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: ***@***.***>
|
|
Sounds great :) |
| @@ -971,9 +1056,13 @@ class _GetItImplementation implements GetIt { | |||
| } | |||
| } | |||
|
|
|||
| final typeRegistration = registrationScope.typeRegistrations | |||
| .putIfAbsent(T, () => _TypeRegistration()); | |||
There was a problem hiding this comment.
I've tested this new feature.
Here we miss the generic type T for it to work 🙂
- .putIfAbsent(T, () => _TypeRegistration());
+ .putIfAbsent(T, () => _TypeRegistration<T>());
| final factories = factoryToRemove.registeredIn.factories; | ||
| if (factories.contains(factoryToRemove)) { | ||
| factories.remove(factoryToRemove); | ||
| if (factories.isEmpty) { |
There was a problem hiding this comment.
As per my understanding line 1273 is the root cause for #358
there is a chance name factory exist but, just factories are empty. in this case factoryToRemove should not be removed.
No description provided.