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

Simplify the web_socket_channel implementation #340

Closed
9 tasks done
brianquinlan opened this issue Apr 5, 2024 · 5 comments
Closed
9 tasks done

Simplify the web_socket_channel implementation #340

brianquinlan opened this issue Apr 5, 2024 · 5 comments

Comments

@brianquinlan
Copy link
Contributor

brianquinlan commented Apr 5, 2024

There are two components:

  1. reimplement web_socket_channel in terms of package:web_socket
  2. remove the WebSocketChannel constructor (the only known usage is in package:shelf_web_socket).

Steps:

  • Update package:shelf_web_socket to not rely on the WebSocketChannel constructor PR
  • Update package:web_socket_channel:
    • implement functionality in terms of package:web_socket PR
    • Remove WebSocketChannel constructor and make that class an interface
  • Publish package:web_socket_channel 3.x (branch: v3_release) PR
  • Update package:shelf_web_socket to accept package:web_socket_channel 3.x (branch: v2_release) PR
  • Publish package:shelf_web_socket 2.x
  • Update package:test to accept package:web_socket_channel 3.x and package:shelf_web_socket 2.x (branch: web_socket_version_constraints) PR
  • Publish package:test
@brianquinlan brianquinlan changed the title Reduce the size of the web_socket_channel implementation Simplify the web_socket_channel implementation Apr 5, 2024
@NotTsunami
Copy link
Contributor

This should actually fix the issue I outlined in #237 (comment). Looking forward to seeing this land.

copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Apr 11, 2024
…_web_socket and web_socket_channel

Bug:dart-lang/web_socket_channel#340
Change-Id: I3dff79a66876005eb77a33f1f82f8ac5d19e764f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/362192
Commit-Queue: Brian Quinlan <bquinlan@google.com>
Reviewed-by: Ben Konyi <bkonyi@google.com>
Reviewed-by: Nate Bosch <nbosch@google.com>
@NotTsunami
Copy link
Contributor

Should there be a corresponding task for build_runner to get updated? It still explicitly relies on 2.x.

@brianquinlan
Copy link
Contributor Author

brianquinlan commented May 7, 2024

@NotTsunami Is build_runner part of a dependency cycle with web_socket_channel? My intent was not to update every package that relies on web_socket_channel, just the ones that make web_socket_channel 3.0 unusable without the update.

@bc-lee
Copy link

bc-lee commented May 8, 2024

I think build_runner is a 1p package, as well as web_socket_channel, so it should be dealt with breaking changes in web_socket_channel by updating build_runner to use the new version of web_socket_channel.

It seems that build_runner and build_daemon use web_socket_channel, but they aren't affected by the breaking change in web_socket_channel 3.0.0 in the library code.

However, build_runner's test code is affected by the breaking change of web_socket_channel 3.0.0. This code uses the removed constructor of WebSocketChannel in web_socket_channel 3.0.0.

Other than this, the following patch seems to be enough to fix the dependency issue in https://github.com/dart-lang/build.

--- a/build_daemon/pubspec.yaml
+++ b/build_daemon/pubspec.yaml
@@ -15,10 +15,10 @@ dependencies:
   path: ^1.8.0
   pool: ^1.5.0
   shelf: ^1.0.0
-  shelf_web_socket: ^1.0.0
+  shelf_web_socket: ">=1.0.0 <3.0.0"
   stream_transform: ^2.0.0
   watcher: ^1.0.0
-  web_socket_channel: ^2.0.0
+  web_socket_channel: ">=2.0.0 <4.0.0"

 dev_dependencies:
   analyzer: '>=3.4.0 <7.0.0'
--- a/build_runner/pubspec.yaml
+++ b/build_runner/pubspec.yaml
@@ -39,12 +39,12 @@ dependencies:
   pub_semver: ^2.0.0
   pubspec_parse: ^1.0.0
   shelf: ^1.0.0
-  shelf_web_socket: ^1.0.0
+  shelf_web_socket: ">=1.0.0 <3.0.0"
   stack_trace: ^1.10.0
   stream_transform: ^2.0.0
   timing: ^1.0.0
   watcher: ^1.0.0
-  web_socket_channel: ^2.0.0
+  web_socket_channel: ">=2.0.0 <4.0.0"
   yaml: ^3.0.0

 dev_dependencies:

@brianquinlan
Copy link
Contributor Author

brianquinlan commented May 9, 2024

@bc-lee Is this blocking you somehow? Could you file an issue with package:builder and CC me?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants