Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Add a WebSocket implementation to package:cupertino_http #1153

Merged
merged 11 commits into from
Mar 19, 2024
Merged

Add a WebSocket implementation to package:cupertino_http #1153

merged 11 commits into from
Mar 19, 2024

Conversation

brianquinlan
Copy link
Collaborator

@brianquinlan brianquinlan commented Mar 8, 2024 •
edited
Loading

This cannot be merge until package:web_socket is available in google3 and has been published.

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

@github-actions github-actions bot added the package:cupertino_http Issues related to package:cupertino_http label Mar 8, 2024
@github-actions github-actions bot added the type-infra A repository infrastructure change or enhancement label Mar 8, 2024
liamappelbe
liamappelbe reviewed Mar 11, 2024
View reviewed changes
pkgs/cupertino_http/lib/src/cupertino_web_socket.dart
final session = URLSession.sessionWithConfiguration(
config ?? URLSessionConfiguration.defaultSessionConfiguration(),
onComplete: (session, task, error) {
if (!readyCompleter.isCompleted) {
Copy link

Choose a reason for hiding this comment

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

This flow is kinda confusing. Can you add some comments?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added some comments and reordered the callbacks into their expected order.

liamappelbe
liamappelbe approved these changes Mar 12, 2024
View reviewed changes
pkgs/cupertino_http/lib/src/cupertino_web_socket.dart
// `onWebSocketTaskOpened should have been called and completed
// `readyCompleter`. So either there was a error creating the connection
// or a logic error.
if (error == null) {
Copy link

Choose a reason for hiding this comment

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

This could be an assert. You can pass a string message as the second arg:
assert(error != null, 'expected an error or "onWebSocketTaskOpened" to be called first');

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 left it this way because assert() isn't a null guard i.e. I'd have to error! on line 74.

Let me know if you'd prefer that.

Copy link

Choose a reason for hiding this comment

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

Oh, right. That makes sense.

@brianquinlan brianquinlan merged commit cfbc191 into dart-lang:master Mar 19, 2024
28 of 30 checks passed
@brianquinlan brianquinlan deleted the cupertino_ws branch March 19, 2024 21:55
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Mar 26, 2024
@devoncarew
[deps] rev crypto, csslib, dartdoc, http, markdown, web
571ec4e 
Revisions updated by `dart tools/rev_sdk_deps.dart`.

crypto (https://github.com/dart-lang/crypto/compare/f059196..69d13c9):
  69d13c9  2024-03-21  Kevin Moore  Switch sha512 to use fastpath with wasm (dart-archive/crypto#165)

csslib (https://github.com/dart-lang/csslib/compare/b58e487..4216525):
  4216525  2024-03-21  Devon Carew  prep for publishing 1.0.1 (dart-archive/csslib#197)

dartdoc (https://github.com/dart-lang/dartdoc/compare/7be9e24..79c1675):
  79c16759  2024-03-25  Parker Lougheed  Migrate client code to package:web (dart-lang/dartdoc#3610)
  0b1c7fa4  2024-03-25  dependabot[bot]  Bump actions/cache from 4.0.1 to 4.0.2 (dart-lang/dartdoc#3734)
  9fe35ec5  2024-03-20  Sam Rawlins  mustachio: Separate out the context stack LUB type calculation (dart-lang/dartdoc#3730)

http (https://github.com/dart-lang/http/compare/5dfea72..7949d6f):
  7949d6f  2024-03-25  Brian Quinlan  cupertino_http: upgrade ffigen version (dart-lang/http#1159)
  051482a  2024-03-22  Brian Quinlan  Ready cupertino_http for release with WebSocket support (dart-lang/http#1158)
  988b4d4  2024-03-20  Brian Quinlan  Prepare package:cronet_http 1.2 for release (dart-lang/http#1157)
  69f4eff  2024-03-20  Hossein Yousefi  [cronet_http] Upgrade jni to 0.7.3 (dart-lang/http#1156)
  d8b1a9e  2024-03-19  Brian Quinlan  Release `package:web_socket` 0.1.0 (dart-lang/http#1155)
  cfbc191  2024-03-19  Brian Quinlan  Add a WebSocket implementation to package:cupertino_http (dart-lang/http#1153)

markdown (https://github.com/dart-lang/markdown/compare/9c6b1af..8d07abc):
  8d07abc  2024-03-19  MJ Studio  Link uri encoding, URL-escaping should be left alone inside the destination (dart-lang/markdown#598)

web (https://github.com/dart-lang/web/compare/4af904f..c522718):
  c522718  2024-03-20  Kevin Moore  Update MDN documentation (dart-lang/web#213)
  f80dcab  2024-03-15  Srujan Gaddam  Update pubspec description to be consistent with README (dart-lang/web#210)
  27936c4  2024-03-15  Devon Carew  Generate api docs for getters (dart-lang/web#207)
  2f13cd5  2024-03-12  Devon Carew  fix unresolved dartdoc links (dart-lang/web#200)
  686827a  2024-03-12  Srujan Gaddam  Remove reference to static interop and point to dart.dev page for JS interop (dart-lang/web#206)
  9b7e29d  2024-03-12  Devon Carew  Add a 'sourced from mdn docs' line to the MDN sourced dartdoc (dart-lang/web#198)
  51e594b  2024-03-05  Srujan Gaddam  Fix dictionary constructors to accept supertype members and create an empty object when there are no fields (dart-lang/web#197)

Change-Id: Ic90c6f5a7e7d701746276031a8028cdfe76bc27a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/359880
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Devon Carew <devoncarew@google.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@liamappelbe liamappelbe liamappelbe approved these changes

Assignees
No one assigned
Labels
package:cupertino_http Issues related to package:cupertino_http type-infra A repository infrastructure change or enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@brianquinlan @liamappelbe