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

[share]: make Share an instantiable object #2818

Open
cedvdb opened this issue Apr 5, 2024 · 4 comments · May be fixed by #3404
Open

[share]: make Share an instantiable object #2818

cedvdb opened this issue Apr 5, 2024 · 4 comments · May be fixed by #3404
Assignees
Labels
enhancement New feature or request triage

Comments

@cedvdb
Copy link

cedvdb commented Apr 5, 2024

Plugin

share_plus

Use case

Currently every method on share are static but they do not need to be. This is less convenient for testing and ioc.

Proposal

Make Share a real object by having its method as instance methods.

@cedvdb cedvdb added enhancement New feature or request triage labels Apr 5, 2024
@miquelbeltran
Copy link
Member

I personally don't see the advantage.

My approach with all 3rd party code, specially when it talks to platform implementations, is to always wrap it in a service class or similar you 100% control. That way, I don't have to worry about static methods, singletons, etc. or possible api changes.

Looking at the other plugins implementations, some allow instances (like DeviceInfo or NetworkInfo) while some use static methods as in Share (like PackageInfo).

At the end of the day, all of them use an internal static singleton, with a factory constructor, e.g.

factory Connectivity() {
_singleton ??= Connectivity._();
return _singleton!;
}

So you may have multiple instances, at the end it is the same singleton.

I'll be open to discuss further, but I don't think it is a priority for us to make this breaking change.

@cedvdb
Copy link
Author

cedvdb commented Apr 5, 2024

My approach with all 3rd party code, specially when it talks to platform implementations, is to always wrap it in a service class or similar you 100% control.

I'm not sure what makes you think I'm not already doing that. However, if I want to test the wrapper, I need to pass each static function in the constructor, which is less convenient than just passing an object. Singletons are also an anti pattern.

I'll be open to discuss further, but I don't think it is a priority for us to make this breaking change.

This is just a "would have been nice to have" for me, no biggie.

@miquelbeltran
Copy link
Member

miquelbeltran commented Apr 6, 2024

Singletons are also an anti pattern.

Sure, but we are talking about a plugin that talks to a platform implementation, not a typical OOP use-case. At the end, the plugin implementation uses a singleton internally.

Something that we could do, is to make all plugins accessible via a .instance static property. e.g. Share.instance.share(). That way, share() won't be a static method, you could still mock the Share class directly, and you can still access it without having to create an instance of the Share class, making the Share constructor private. I think this pattern is found in all the Flutter Fire plugins.

This is something that could be done with the upcoming Share cleanup: #2819

@miquelbeltran miquelbeltran self-assigned this Apr 10, 2024
@miquelbeltran miquelbeltran linked a pull request Dec 20, 2024 that will close this issue
16 tasks
@miquelbeltran
Copy link
Member

FYI, this is now being done in #3404

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request triage
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants