search

dart-lang / http

A composable API for making HTTP requests in Dart.
https://pub.dev/packages/http
BSD 3-Clause "New" or "Revised" License
1.02k stars 353 forks source link

Add a note saying that we only create a single `Client`. #1234

Closed brianquinlan closed 3 months ago

brianquinlan commented 3 months ago

There was some confusion about the semantics raised in https://github.com/dart-lang/http/issues/422

Contribution guidelines:
- See our [contributor guide](https://github.com/dart-lang/.github/blob/main/CONTRIBUTING.md) for general expectations for PRs. - Larger or significant changes should be discussed in an issue before creating a PR. - Contributions to our repos should follow the [Dart style guide](https://dart.dev/guides/language/effective-dart) and use `dart format`. - Most changes should add an entry to the changelog and may need to [rev the pubspec package version](https://github.com/dart-lang/sdk/blob/main/docs/External-Package-Maintenance.md#making-a-change). - Changes to packages require [corresponding tests](https://github.com/dart-lang/.github/blob/main/CONTRIBUTING.md#Testing). Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.
natebosch commented 3 months ago

Can you expand this? Maybe add a bit more detail in the comment in source, and elaborate further in the CL description if necessary.

With the current comment, my first assumption was that it means http_factory.httpClient() returns a singleton instance and I was confused by the naming.

I think what it actually means is Provider<Client> is a singleton provider, and it only calls it's create argument once. Is that correct? I don't know anything about Provider, and I don't think we should assume the reader does either. If we're describing some property of that library I think we should make it explicit.

brianquinlan commented 3 months ago

Can you expand this? Maybe add a bit more detail in the comment in source, and elaborate further in the CL description if necessary.

With the current comment, my first assumption was that it means http_factory.httpClient() returns a singleton instance and I was confused by the naming.

I think what it actually means is Provider<Client> is a singleton provider, and it only calls it's create argument once. Is that correct? I don't know anything about Provider, and I don't think we should assume the reader does either. If we're describing some property of that library I think we should make it explicit.

You're right. I've updated the comment. Is it more clear now?