Implement new methods for GetIt service locator

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 148 forks source link

Implement new methods for GetIt service locator #383

Closed iv4n-ga6l closed 3 weeks ago

iv4n-ga6l commented 1 month ago

Access Count Tracking : We can now track how many times a singleton has been accessed using the getAccessCount method.
Clear All Instances : The clearAllInstances method allows clearing all instances of a specific type, including named instances.
Set Default Instance : The setDefault method allows setting or replacing the default instance for a type, with an option to use a named instance.

escamoteur commented 1 month ago

HI, I'm in general happy about contributions to get_it but before submitting an PR with new functions it would be good to open an issue outlining the purpose of the new functions.

I can see some value to get the accessnumber for a certain type while debugging. does it have any meaningful application in an production app?
clearAll instances: this would be better implemented by extending the unregister function with an fromAllScopes parameter
setDefaultInstance: this is misleading because in reality it only replaces an instance and it only works for singletons. So it would probably better be named as replaceSingletonInstance

cheers Thomas

iv4n-ga6l commented 1 month ago

@escamoteur Ah sorry, I had to open an issue and make the proposal first before submitting my PR 🙂.

I will make some adjustments regarding your suggestions.

iv4n-ga6l commented 1 month ago

I've made some adjustments:

Access count: You're right that the access count might not have significant value in a production app. I've kept it, but mark it as a debug-only feature.
Clearing all instances: Extending the unregister function with a fromAllScopes parameter is a more flexible and consistent approach.
Renaming setDefault to replaceSingletonInstance: The name replaceSingletonInstance more accurately describes what the function does.

escamoteur commented 1 month ago

It might be a good idea to wrap the code that updates the count inside an assert so it will be removed in a production app Am 23. Okt. 2024, 16:07 +0100 schrieb Ivan APEDO @.***>:

I've made some adjustments:

• Access count: You're right that the access count might not have significant value in a production app. I've kept it, but mark it as a debug-only feature. • Clearing all instances: Extending the unregister function with a fromAllScopes parameter is a more flexible and consistent approach. • Renaming setDefault to replaceSingletonInstance: The name replaceSingletonInstance more accurately describes what the function does.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

iv4n-ga6l commented 1 month ago

@escamoteur I check here the debug mode before updating the count. Should I put this logic inside the assert ?

escamoteur commented 1 month ago

Checking for debug mode will execute that check on every call in release. So wrapping the increment inside an asserts bool test condition (make the increment a function that returns true) will completely remove it in release builds Am 23. Okt. 2024, 20:30 +0100 schrieb Ivan APEDO @.***>:

@escamoteur I check here the debug mode before updating the count. Should I put this logic inside the assert ? image.png (view on web) — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

escamoteur commented 1 month ago

don't know why but your latest commit seems to touch a lot of lines which makes it difficult to follow what you changed. And idea why this is?

iv4n-ga6l commented 1 month ago

Only the get_it.dart, get_it_impl.dart and get_it_test.dart files were modified. I did not touch any other files.

escamoteur commented 1 month ago

Yes, but if you look at the diff of the implementation.dart it's really a lot of changes. Will need more time to carefully review them Am 27. Okt. 2024, 14:31 +0000 schrieb Ivan APEDO @.***>:

Only the get_it.dart, get_it_impl.dart and get_it_test.dart files were modified. I did not touch any other files. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

iv4n-ga6l commented 1 month ago

I didn't change much. I just modified the new methods that I implemented myself and added a new parameter to the unregister method.

escamoteur commented 1 month ago

Sorry Ivan, I was super busy these days Nachricht von Ivan APEDO @.***> am Nov 6, 2024, 6:45 PM +0100:

@IvanGael requested your review on: #383 Implement new methods for GetIt service locator. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because your review was requested.Message ID: @.***>

escamoteur commented 3 weeks ago

will do some final fixes

escamoteur commented 3 weeks ago

please check my changes in https://github.com/fluttercommunity/get_it/tree/incoming I think this is an easier way to implement the unregister fromAllScopes