[cronet_http] 🏗️ Use dart-define to determine dependency by AlexV525 · Pull Request #1111 · dart-lang/http

AlexV525 · 2024-01-10T02:30:33Z

Instead of creating a new embedded package, we can use dart-define to conditionally implement dependencies.

I’ve reviewed the contributor guide and applied the relevant portions to this PR.

Contribution guidelines:

See our contributor guide 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 and use dart format.
Most changes should add an entry to the changelog and may need to rev the pubspec package version.
Changes to packages require corresponding tests.

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

AlexV525 · 2024-01-10T03:49:01Z

Require these PRs to land first:

AlexV525 · 2024-01-10T05:14:54Z

This also includes #1101.

# Conflicts: # .github/workflows/cronet.yml # pkgs/cronet_http/android/build.gradle # pkgs/cronet_http/example/android/app/build.gradle # pkgs/cronet_http/tool/prepare_for_embedded.dart

brianquinlan

This is a really cool approach! I think that you need to increment the pub spec version and add an entry to the CHANGELOG.

One this is live, I think that I can add a note to cronet_http_embedded and deprecate it.

brianquinlan · 2024-02-08T02:04:25Z

-        userAgent: 'Book Agent');
+      cacheMode: CacheMode.memory,
+      cacheMaxSize: 2 * 1024 * 1024,
+       userAgent: 'Book Agent',


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

Flutter prefers trailing commas, which makes the formatting different.

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

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

brianquinlan · 2024-02-08T02:12:07Z


+### Use embedded Cronet
+
+The embedded [Cronet][] is provided as `org.chromium.net:cronet-embedded`


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:
...

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.

brianquinlan · 2024-02-08T02:22:23Z

+For example:
+
+```
+flutter run --dart-define=cronetHttpNoPlay=true


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

cronet_http_no_play=true

Any guide suggesting this?

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.

brianquinlan · 2024-02-08T02:35:00Z

After patching this in and using it for a bit, I'm even more amazed by this approach!

AlexV525 · 2024-02-08T03:02:34Z

@brianquinlan Thanks for the review. I'll update this after #1091 gets merged.

# Conflicts: # .github/workflows/cronet.yml # pkgs/cronet_http/README.md

brianquinlan · 2024-02-23T17:28:51Z

-        userAgent: 'Book Agent');
+      cacheMode: CacheMode.memory,
+      cacheMaxSize: 2 * 1024 * 1024,
+       userAgent: 'Book Agent',


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

brianquinlan · 2024-02-23T17:31:45Z

+For example:
+
+```
+flutter run --dart-define=cronetHttpNoPlay=true


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.

brianquinlan · 2024-02-23T21:11:50Z

Thanks!

Revisions updated by `dart tools/rev_sdk_deps.dart`. http (https://github.com/dart-lang/http/compare/ce0de37..6e0a46f): 6e0a46f 2024-02-24 Alex Li [cronet_http] 🏗️ Use dart-define to determine dependency (dart-lang/http#1111) d5cab74 2024-02-23 Alex Li 👷 Update configuration related files (dart-lang/http#1091) 6873731 2024-02-22 Brian Quinlan Make it possible for `CronetClient` to own the lifetime of an existing `CronetEngine` (dart-lang/http#1137) protobuf (https://github.com/dart-lang/protobuf/compare/f085bfd..ef0ab7d): ef0ab7d 2024-02-23 Sebastian Optimize repeated field decoding (google/protobuf.dart#911) tools (https://github.com/dart-lang/tools/compare/9f4e6a4..c7fbf26): c7fbf26 2024-02-21 Elias Yishak Fallback to current datetime when failing on parse session json file (dart-lang/tools#235) web (https://github.com/dart-lang/web/compare/975e55c..d96c01d): d96c01d 2024-02-23 Srujan Gaddam Use extension types and generics internally (dart-lang/web#184) yaml_edit (https://github.com/dart-lang/yaml_edit/compare/82ab64d..54884db): 54884db 2024-02-23 Devon Carew update to the latest sdk; update lints (dart-archive/yaml_edit#68) Change-Id: Ib2cb7971df05f4501f81fe5c04b7eaf88d0d93a3 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/354381 Commit-Queue: Devon Carew <devoncarew@google.com> Reviewed-by: Konstantin Shcheglov <scheglov@google.com>

AlexV525 added 3 commits January 10, 2024 10:16

🏗️ Use dart-define to determine dependency

c8b5ed9

💚 Update workflow

65b92c7

💚 --

43a59ae

github-actions Bot added package:cronet_http type-infra A repository infrastructure change or enhancement labels Jan 10, 2024

AlexV525 added 5 commits January 10, 2024 10:43

Update cronet.yml

e524a7a

🔊 ++

208a647

Update cronet.yml

cabf835

💚 --

5d525f5

✅ --

df4033f

AlexV525 added 3 commits January 10, 2024 13:21

🎨 Format

32e2801

Update README.md

1339348

Merge branch 'master' into feat/dart-define-embedded

13849c2

# Conflicts: # .github/workflows/cronet.yml # pkgs/cronet_http/android/build.gradle # pkgs/cronet_http/example/android/app/build.gradle # pkgs/cronet_http/tool/prepare_for_embedded.dart

AlexV525 mentioned this pull request Jan 23, 2024

package:cronet_http_embedded needs a new name and publisher #1105

Closed

brianquinlan requested changes Feb 8, 2024

View reviewed changes

brianquinlan reviewed Feb 8, 2024

View reviewed changes

AlexV525 mentioned this pull request Feb 20, 2024

Error: Exception in Java code called through JNI: java.lang.RuntimeException: All available Cronet providers are disabled. A provider should be enabled before it can be used. cfug/dio#2116

Closed

AlexV525 added 3 commits February 23, 2024 09:40

Merge branch 'master' into feat/dart-define-embedded

dec296b

# Conflicts: # .github/workflows/cronet.yml # pkgs/cronet_http/README.md

📝 Update README

4c000c3

🔖 1.2.0-wip

e1df37c

AlexV525 requested a review from brianquinlan February 23, 2024 02:07

brianquinlan reviewed Feb 23, 2024

View reviewed changes

🎨 --

d623b3d

AlexV525 requested a review from brianquinlan February 23, 2024 17:38

AlexV525 marked this pull request as ready for review February 23, 2024 17:38

brianquinlan approved these changes Feb 23, 2024

View reviewed changes

Comment thread pkgs/cronet_http/CHANGELOG.md

brianquinlan merged commit 6e0a46f into dart-lang:master Feb 23, 2024

AlexV525 deleted the feat/dart-define-embedded branch February 23, 2024 23:09

vanyasem mentioned this pull request Jul 14, 2024

Concerns about the usage of --dart-define in gradle scripts #1267

Open


		### Use embedded Cronet

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

Conversation

AlexV525 commented Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexV525 commented Jan 10, 2024

Uh oh!

AlexV525 commented Jan 10, 2024

Uh oh!

brianquinlan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brianquinlan commented Feb 8, 2024

Uh oh!

AlexV525 commented Feb 8, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brianquinlan commented Feb 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AlexV525 commented Jan 10, 2024 •

edited

Loading