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

Jump to bottom

pkgs/ok_http: JNIgen fixes and added WebSocket support #1257

Merged
merged 29 commits into from
Jul 17, 2024
Merged

pkgs/ok_http: JNIgen fixes and added WebSocket support #1257

merged 29 commits into from
Jul 17, 2024

Conversation

Anikate-De
Copy link
Contributor

Items in this PR:

  • Update package: jni, jnigen versions
  • Several jnigen usage fixes (thanks to @HosseinYousefi)
  • Generate JNI bindings for WebSocket support required classes
  • Implement WebSocket
  • Pass all conformance tests in package: web_socket_conformance_tests
  • Modify the example app to showcase both HTTP and WebSocket side-by-side
  • Added Docs
  • Updated README.md to boast new WebSocket support
  • Updated CHANGELOG.md
  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

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

Anikate-De and others added 13 commits June 30, 2024 22:58
@Anikate-De
ok_http: upgrade jni and regenerate bindings
dbf7d1d
@Anikate-De @HosseinYousefi
ok_http: jnigen fixes
bd97d77 
Co-authored-by: Hossein Yousefi <[email protected]>
@Anikate-De
generate bindings for okhttp websocket classes
4cb89e8
@Anikate-De
add websocket dependency
daf5d3a
@Anikate-De
create proxy and interceptor; generate bindings
87f7595
@Anikate-De
first websocket impl
dc6984b
@Anikate-De
create an example section for websocket demo
78b6637
@Anikate-De
add integration tests
7b93fb0
@Anikate-De
add helper comments and docs to kotlin files
bb68b2b
@Anikate-De
add more docs
5cea12b
@Anikate-De
Squashed commit of the following:
3c14e5d 
commit 4178b67
Author: Brian Quinlan <[email protected]>
Date:   Wed Jul 3 17:16:38 2024 -0700

    Clarify when Client.close must be called (dart-lang#1255)

commit 719dc5f
Author: Brian Quinlan <[email protected]>
Date:   Tue Jul 2 11:18:29 2024 -0700

    Upgrade to http_image_provider: 0.0.3 (dart-lang#1253)

commit 75b1efb
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Tue Jul 2 10:46:54 2024 -0700

    Bump the github-actions group across 1 directory with 4 updates (dart-lang#1251)

    Bumps the github-actions group with 4 updates in the / directory: [actions/checkout](https://github.com/actions/checkout), [subosito/flutter-action](https://github.com/subosito/flutter-action), [reactivecircus/android-emulator-runner](https://github.com/reactivecircus/android-emulator-runner) and [dart-lang/setup-dart](https://github.com/dart-lang/setup-dart).

    Updates `actions/checkout` from 4.1.1 to 4.1.7
    - [Release notes](https://github.com/actions/checkout/releases)
    - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
    - [Commits](actions/checkout@b4ffde6...692973e)

    Updates `subosito/flutter-action` from 2.12.0 to 2.16.0
    - [Release notes](https://github.com/subosito/flutter-action/releases)
    - [Commits](subosito/flutter-action@2783a3f...44ac965)

    Updates `reactivecircus/android-emulator-runner` from 2.30.1 to 2.31.0
    - [Release notes](https://github.com/reactivecircus/android-emulator-runner/releases)
    - [Changelog](https://github.com/ReactiveCircus/android-emulator-runner/blob/main/CHANGELOG.md)
    - [Commits](ReactiveCircus/android-emulator-runner@6b0df4b...77986be)

    Updates `dart-lang/setup-dart` from 1.6.4 to 1.6.5
    - [Release notes](https://github.com/dart-lang/setup-dart/releases)
    - [Changelog](https://github.com/dart-lang/setup-dart/blob/main/CHANGELOG.md)
    - [Commits](dart-lang/setup-dart@f0ead98...0a8a0fc)

    ---
    updated-dependencies:
    - dependency-name: actions/checkout
      dependency-type: direct:production
      update-type: version-update:semver-patch
      dependency-group: github-actions
    - dependency-name: subosito/flutter-action
      dependency-type: direct:production
      update-type: version-update:semver-minor
      dependency-group: github-actions
    - dependency-name: reactivecircus/android-emulator-runner
      dependency-type: direct:production
      update-type: version-update:semver-minor
      dependency-group: github-actions
    - dependency-name: dart-lang/setup-dart
      dependency-type: direct:production
      update-type: version-update:semver-patch
      dependency-group: github-actions
    ...

    Signed-off-by: dependabot[bot] <[email protected]>
    Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

commit cdfb94c
Author: Brian Quinlan <[email protected]>
Date:   Mon Jul 1 14:16:10 2024 -0700

    Add an section explaining the benefits of using `package:ok_http`. (dart-lang#1252)
@Anikate-De
add more and more docs
91c9378
@Anikate-De
update changelog
9968a76
@google-cla Google CLA
Copy link

google-cla bot commented Jul 7, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@github-actions github-actions bot added package:cupertino_http Issues related to package:cupertino_http package:http package:cronet_http type-infra A repository infrastructure change or enhancement labels Jul 7, 2024
@github-actions github-actions bot removed package:cupertino_http Issues related to package:cupertino_http package:http package:cronet_http type-infra A repository infrastructure change or enhancement labels Jul 7, 2024
@Anikate-De Anikate-De changed the title Okhttp websockets pkgs/ok_http: JNIgen fixes and added WebSocket support Jul 7, 2024
@Anikate-De
Copy link
Contributor Author

Requesting for review by pinging as I don't have permissions
@brianquinlan @camsim99 @HosseinYousefi

@Anikate-De Anikate-De mentioned this pull request Jul 14, 2024
Open
camsim99
camsim99 approved these changes Jul 15, 2024
View reviewed changes
Copy link

@camsim99 camsim99 left a comment

Choose a reason for hiding this comment

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

LGTM! Just left some nits

pkgs/ok_http/README.md Show resolved Hide resolved
pkgs/ok_http/README.md Show resolved Hide resolved
pkgs/ok_http/lib/src/ok_http_web_socket.dart Outdated Show resolved Hide resolved
@Anikate-De
Copy link
Contributor Author

@camsim99, should I change the version from ok_http: 0.1.0-wip to 0.1.0?

@camsim99
Copy link

@camsim99, should I change the version from ok_http: 0.1.0-wip to 0.1.0?

Ah yes because this finishes the implementation.

@Anikate-De
Copy link
Contributor Author

@camsim99, should I change the version from ok_http: 0.1.0-wip to 0.1.0?

Ah yes because this finishes the implementation.

Done! Ready for release 🎉

@camsim99
Copy link

Done! Ready for release 🎉

Woohoo!! Looks like something with the CLA happened and I think @HosseinYousefi has to approve, but in the meantime I will follow up on #1259

@github-actions github-actions bot added the type-infra A repository infrastructure change or enhancement label Jul 15, 2024
@HosseinYousefi
Copy link
Member

I accepted the CLA with my other email. In general, no need to add me as a co-author when I review and if you do make sure it's my google email. Thanks!

HosseinYousefi
HosseinYousefi reviewed Jul 16, 2024
View reviewed changes
pkgs/ok_http/pubspec.yaml Outdated Show resolved Hide resolved
pkgs/ok_http/lib/src/ok_http_web_socket.dart Outdated Show resolved Hide resolved
HosseinYousefi
HosseinYousefi approved these changes Jul 16, 2024
View reviewed changes
Copy link
Member

@HosseinYousefi HosseinYousefi left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @Anikate-De!

@HosseinYousefi
Copy link
Member

I dismissed @brianquinlan's review as he's OOO. @camsim99 Feel free to merge this whenever you see fit.

@camsim99
Copy link

It appears I do not have write access :/ Will need to get this resolved to merge this.

@natebosch natebosch merged commit a0781c5 into dart-lang:master Jul 17, 2024
29 checks passed
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Jul 19, 2024
@devoncarew
[deps] rev dartdoc, http, sse, tools
29091ee 
Revisions updated by `dart tools/rev_sdk_deps.dart`.

dartdoc (https://github.com/dart-lang/dartdoc/compare/1d00523..5230f8f):
  5230f8fd  2024-07-16  Srujan Gaddam  Add support for package:web v1.0.0 (dart-lang/dartdoc#3821)

http (https://github.com/dart-lang/http/compare/757438e..a0781c5):
  a0781c5  2024-07-18  Anikate De  pkgs/ok_http: JNIgen fixes and added WebSocket support (dart-lang/http#1257)

sse (https://github.com/dart-lang/sse/compare/52d042f..af2c5c5):
  af2c5c5  2024-07-17  Kevin Moore  Bump to latest lints (dart-lang/sse#114)
  a6ae8dc  2024-07-16  Srujan Gaddam  Add support for package:web v1.0.0 (dart-lang/sse#113)

tools (https://github.com/dart-lang/tools/compare/6a07a16..55dbd6e):
  55dbd6e  2024-07-18  Devon Carew  rev to 2.3.2 in prep for publishing (dart-lang/tools#287)
  f957dd2  2024-07-17  Konstantin Scheglov  Stop using deprecated ContextLocator and ContextBuilder. (dart-lang/tools#285)

Change-Id: I6f51418c424a3b1400ab245f557a53e92da44848
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/376700
Reviewed-by: Konstantin Shcheglov <[email protected]>
Commit-Queue: Devon Carew <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camsim99 camsim99 camsim99 approved these changes

@HosseinYousefi HosseinYousefi HosseinYousefi approved these changes

@brianquinlan brianquinlan Awaiting requested review from brianquinlan

Assignees
No one assigned
Labels
type-infra A repository infrastructure change or enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Anikate-De @camsim99 @HosseinYousefi @brianquinlan @natebosch