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

Update get_it_impl.dart to dispose in reverse order #350

Closed bvoq closed 11 months ago

bvoq commented 1 year ago

There are a few situations, where one instance depends on an instance you created before. In all of these situations, it makes more sense to dispose of the widgets in reverse order.

escamoteur commented 1 year ago

Hey, after reading the docs of the linked hash map it seems that that would solve the problem without hurting performance. Good idea Cheers Thomas Am 22. Nov. 2023, 10:00 +0100 schrieb Kevin De Keyser @.***>:

@bvoq commented on this pull request. In lib/get_it_impl.dart:

@@ -309,7 +309,7 @@ class _Scope {

Future reset({required bool dispose}) async { if (dispose) {

  • for (final factory in allFactories) {
  • for (final factory in allFactories.reversed) { Perhaps an easier way would be to use LinkedHashMap instead, which is order-preserving by default. Would you be interested in such a PR? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: @.***>
bvoq commented 11 months ago

Alright I've added tests, one of which fails with the current version but not with this new version.

Minor note: I wouldn't rename the type of factoriesByName, since: final factoriesByName = <String?, Map<Type, _ServiceFactory<Object, dynamic, dynamic>>>{}; is of type:

Map<String?, Map<Type, _ServiceFactory<Object, dynamic, dynamic>>>

which is not of subtype:

// ignore: prefer_collection_literals
final factoriesByName = LinkedHashMap<String?,LinkedHashMap<Type, _ServiceFactory<Object, dynamic, dynamic>>>();

Without renaming the type, this could be released as a minor update. If we rename the type, it might break the clients and would require a major version update.

I'm ok with it either way, do you want to edit the CHANGELOG.md with the correct date?

Thanks for looking at the PR!

escamoteur commented 11 months ago

Why do you think a change of the type of this this internal data structure might break any apps that use get_it? Change log addition would be fine. Also do we already have a note in the docs pointing out that we now dispose in reverse order?

bvoq commented 11 months ago

Why do you think a change of the type of this this internal data structure might break any apps that use get_it?

I was wrong, my bad. I didn't see that it was part of a private class _Scope. I've changed the type now and made the changelog more explicit. I've also added information to the docs pointing out that they are disposed in the reverse order.

escamoteur commented 11 months ago

thanks a lot for this fine PR, I just published a new version