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.

Sign up for GitHub

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

Document that runWithClient should not be used with Flutter #1249

Merged
merged 1 commit into from
Jun 27, 2024
Merged

Document that runWithClient should not be used with Flutter #1249

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 11 additions & 19 deletions pkgs/http/lib/src/client.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,34 +202,26 @@ Client? get zoneClient {
/// HTTP requests made using `dart:io` or `dart:html` APIs,
/// or using specifically instantiated client implementations, are not affected.
///
/// When used in the context of Flutter, [runWithClient] should be called before
/// [`WidgetsFlutterBinding.ensureInitialized`](https://api.flutter.dev/flutter/widgets/WidgetsFlutterBinding/ensureInitialized.html)
/// because Flutter runs in whatever [Zone] was current at the time that the
/// bindings were initialized.
///
/// [`runApp`](https://api.flutter.dev/flutter/widgets/runApp.html) calls
/// [`WidgetsFlutterBinding.ensureInitialized`](https://api.flutter.dev/flutter/widgets/WidgetsFlutterBinding/ensureInitialized.html)
/// so the easiest approach is to call that in [body]:
/// ```
/// void main() {
/// var clientFactory = Client.new; // Constructs the default client.
/// if (Platform.isAndroid) {
/// clientFactory = MyAndroidHttpClient.new;
/// }
/// runWithClient(() => runApp(const MyApp()), clientFactory);
/// }
/// ```
///
/// If [runWithClient] is used and the environment defines
/// `no_default_http_client=true` then generated binaries may be smaller e.g.
/// ```shell
/// $ flutter build appbundle --dart-define=no_default_http_client=true ...
/// $ dart compile exe --define=no_default_http_client=true ...
/// ```
///
/// If `no_default_http_client=true` is set then any call to the [Client]
/// factory (i.e. `Client()`) outside of the [Zone] created by [runWithClient]
/// will throw [StateError].
///
/// > [!IMPORTANT]
/// > Flutter does not guarantee that callbacks are executed in a particular
/// > [Zone].
/// >
/// > Instead of using [runWithClient], Flutter developers can use a framework,
/// > such as [`package:provider`](https://pub.dev/packages/provider), to make
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does package:http_client_image_provider work with package:provider?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, we even have an usage example in the demo app:

http/pkgs/flutter_http_example/lib/main.dart

Line 141 in bf96551

image: HttpImage(

The name HttpImage used by package:http_client_image_provider seems a bit unfortunate to me. I wonder if proposing to rename it to HttpClientImageProvider upstream would be worth it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have an opinion on renaming - but this doc change LGTM given that usage in the example.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maintainer of package:http_image_provider here: I don't mind the proposed name change. It should be pretty straightforward to change anyway. Just rename it, create an alias for the old name and mark the alias as deprecated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started working on a package:http_image_provider PR yesterday but then I thought that the naming confusion is probably mostly caused by the fact that the Image widget calls its ImageProvider argument image.

So you have:

Image(image: HttpImage(...))  // Looks like we are nesting an images.

I guess that:

Image(image: HttpImageProvider(...)) // Slightly better?

is slightly better but really, the image parameter should be called something like provider so it reads like:

Image(provider: ...) // Now I actually understand that we are provider the data here.

it is probably still worth doing though and maybe I'll file a flutter issue as well

/// > a [Client] available throughout their applications.
/// >
/// > See the
/// > [Flutter Http Example](https://github.com/dart-lang/http/tree/master/pkgs/flutter_http_example).
R runWithClient<R>(R Function() body, Client Function() clientFactory,
{ZoneSpecification? zoneSpecification}) =>
runZoned(body,
Expand Down