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

[cronet_http] šŸ—ļø Use dart-define to determine dependency #1111

Merged
merged 15 commits into from
Feb 23, 2024
18 changes: 6 additions & 12 deletions .github/workflows/cronet.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,10 @@ jobs:
runs-on: macos-latest
strategy:
matrix:
package: ['cronet_http', 'cronet_http_embedded']
cronetHttpNoPlay: ['false', 'true']
defaults:
run:
working-directory: pkgs/cronet_http
steps:
- uses: actions/checkout@v4
- uses: actions/setup-java@v4
Expand All @@ -36,22 +39,13 @@ jobs:
- uses: subosito/flutter-action@v2
with:
channel: 'stable'
- name: Make cronet_http_embedded copy
if: ${{ matrix.package == 'cronet_http_embedded' }}
run: |
mv pkgs/cronet_http pkgs/cronet_http_embedded
cd pkgs/cronet_http_embedded
flutter pub get && dart tool/prepare_for_embedded.dart
- id: install
name: Install dependencies
working-directory: 'pkgs/${{ matrix.package }}'
run: flutter pub get
- name: Check formatting
working-directory: 'pkgs/${{ matrix.package }}'
run: dart format --output=none --set-exit-if-changed .
if: always() && steps.install.outcome == 'success'
- name: Analyze code
working-directory: 'pkgs/${{ matrix.package }}'
run: flutter analyze --fatal-infos
if: always() && steps.install.outcome == 'success'
- name: Run tests
Expand All @@ -64,6 +58,6 @@ jobs:
# - pkgs/cronet_http/example/android/app/build.gradle
api-level: 21
arch: x86_64
target: ${{ matrix.package == 'cronet_http_embedded' && 'default' || 'google_apis' }}
target: ${{ matrix.cronetHttpNoPlay == 'true' && 'default' || 'google_apis' }}
profile: pixel
script: cd 'pkgs/${{ matrix.package }}/example' && flutter test --timeout=1200s integration_test/
script: cd pkgs/cronet_http/example && flutter test --dart-define=cronetHttpNoPlay=${{ matrix.cronetHttpNoPlay }} --timeout=1200s integration_test/
55 changes: 38 additions & 17 deletions pkgs/cronet_http/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,20 @@
[![package publisher](https://img.shields.io/pub/publisher/cronet_http.svg)](https://pub.dev/packages/cronet_http/publisher)

An Android Flutter plugin that provides access to the
[Cronet][]
HTTP client.
[Cronet][] HTTP client.

Cronet is available as part of
[Google Play Services][].
Cronet is available as part of [Google Play Services][]
and as [a standalone embedded library][].

This package depends on [Google Play Services][] for its [Cronet][]
implementation.
[`package:cronet_http_embedded`](https://pub.dev/packages/cronet_http_embedded)
is functionally identical to this package but embeds [Cronet][] directly
instead of relying on [Google Play Services][].
This package depends on [Google Play Services][]
for its [Cronet][] implementation.
To use the embedded version of [Cronet][] without [Google Play Services][],
see [Use embedded Cronet](#use-embedded-cronet).

## Motivation

Using [Cronet][], rather than the socket-based [dart:io HttpClient][]
implemententation, has several advantages:
Using [Cronet][], rather than the socket-based
[dart:io HttpClient][] implementation, has several advantages:

1. It automatically supports Android platform features such as HTTP proxies.
2. It supports configurable caching.
Expand All @@ -40,22 +38,45 @@ void main() async {
final Client httpClient;
if (Platform.isAndroid) {
final engine = CronetEngine.build(
cacheMode: CacheMode.memory,
cacheMaxSize: 2 * 1024 * 1024,
userAgent: 'Book Agent');
cacheMode: CacheMode.memory,
cacheMaxSize: 2 * 1024 * 1024,
userAgent: 'Book Agent',
Copy link
Collaborator

Choose a reason for hiding this comment

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

There seems to be some random formatting changes in this file...isn't this formatting actually incorrect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Flutter prefers trailing commas, which makes the formatting different.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This just looks misindented though. Can you change it back?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, the extra space. Would it be accepted if I only took it away?

);
httpClient = CronetClient.fromCronetEngine(engine);
} else {
httpClient = IOClient(HttpClient()..userAgent = 'Book Agent');
}

final response = await client.get(Uri.https(
final response = await client.get(
Uri.https(
'www.googleapis.com',
'/books/v1/volumes',
{'q': 'HTTP', 'maxResults': '40', 'printType': 'books'}));
{'q': 'HTTP', 'maxResults': '40', 'printType': 'books'},
),
);
}
```

### Use embedded Cronet

The embedded [Cronet][] is provided as `org.chromium.net:cronet-embedded`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe something more explicit like:

By default, package:cronet_http uses the [Cronet][] library provided by [Google Play Services][].

If you want your application to work without [Google Play Services][], you can instead depend on the org.chromium.net:cronet-embedded package by using dart-define to set cronetHttpNoPlay is set to true.

For example:
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By default, package:cronet_http uses the [Cronet][] library provided by [Google Play Services][].

The first paragraph duplicates the previous paragraphs. I'll take the second.

which does not require to use the [Google Play Services][].
The library uses `dart-define` to switch to the embedded implementation
when `cronetHttpNoPlay` is set to `true`.
For example:

```
flutter run --dart-define=cronetHttpNoPlay=true
Copy link
Collaborator

Choose a reason for hiding this comment

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

The convention seems to be to use name_with_underscore. So maybe:

cronet_http_no_play=true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any guide suggesting this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, I just looked at some existing defines e.g. test_runner.configuration.

But it is not consistent so feel free to leave it this way.

```

To use the embedded version in `flutter test`:

```
flutter test --dart-define=cronetHttpNoPlay=true
```

[Cronet]: https://developer.android.com/guide/topics/connectivity/cronet/reference/org/chromium/net/package-summary
[dart:io HttpClient]: https://api.dart.dev/stable/dart-io/HttpClient-class.html
[Google Play Services]: https://developers.google.com/android/guides/overview
[a standalone embedded library]: https://mvnrepository.com/artifact/org.chromium.net/cronet-embedded
[dart:io HttpClient]: https://api.dart.dev/stable/dart-io/HttpClient-class.html
[package:http Client]: https://pub.dev/documentation/http/latest/http/Client-class.html
12 changes: 0 additions & 12 deletions pkgs/cronet_http/README_EMBEDDED.md

This file was deleted.

17 changes: 16 additions & 1 deletion pkgs/cronet_http/android/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,17 @@ rootProject.allprojects {
}
}

def dartDefines = [
cronetHttpNoPlay: 'false'
]
if (project.hasProperty('dart-defines')) {
def defines = project.property('dart-defines').split(',').collectEntries { entry ->
def pair = new String(entry.decodeBase64(), 'UTF-8').split('=')
[(pair.first()): pair.last()]
}
dartDefines = dartDefines + defines
}

apply plugin: 'com.android.library'
apply plugin: 'kotlin-android'

Expand Down Expand Up @@ -65,5 +76,9 @@ android {
}

dependencies {
implementation "com.google.android.gms:play-services-cronet:18.0.1"
if (dartDefines.cronetHttpNoPlay == 'true') {
implementation 'org.chromium.net:cronet-embedded:113.5672.61'
} else {
implementation "com.google.android.gms:play-services-cronet:18.0.1"
}
}
162 changes: 0 additions & 162 deletions pkgs/cronet_http/tool/prepare_for_embedded.dart

This file was deleted.