dart-lang / http

A composable API for making HTTP requests in Dart.
https://pub.dev/packages/http
BSD 3-Clause "New" or "Revised" License
1.03k stars 359 forks source link

CupertinoWebSocket: Allow close codes as defined in RFC 6455 #1294

Open kpsroka opened 3 months ago

kpsroka commented 3 months ago

This is a follow-up from issue https://github.com/dart-lang/http/issues/1203.

At this moment, the CupertinoWebSocket.close will throw if passed non-null code parameter that's not equal to 1000 or in [3000, 4999] range.

This is not in line with the RFC 6455 specification, that allows additional codes in range 1000-1010 for client use.

The WHATWG specification of the WebSocket close operation is less permissive, but since:

I believe that it should permit all client-side codes permitted by RFC 6455.

I'm fine with providing a PR for applying this change.

brianquinlan commented 3 months ago

Hey @kpsroka

In chose the current subset of close codes because they are supported by all WebSocket implementations that I'm aware of.

Maybe it would make sense to modify the conformance tests to allow implementations to support additional close codes: https://github.com/dart-lang/http/blob/master/pkgs/web_socket_conformance_tests/lib/src/close_local_tests.dart

We could change the documentation for WebSocket.close to be something like:

  /// Closes the WebSocket connection and the [events] `Stream`.
  ///
  /// Sends a Close frame to the peer. If the optional [code] and [reason]
  /// arguments are given, they will be included in the Close frame. If no
  /// [code] is set then the peer will see a 1005 status code. If no [reason]
  /// is set then the peer will not receive a reason string.
  ///
- /// Throws an [ArgumentError] if [code] is not 1000 or in the range 3000-4999.
+ /// Throws an [ArgumentError] if the value of [code] is not supported. All implementations
+ /// support the value 1000 and values in the range 3000-4999.
  ...
  Future<void> close([int? code, String? reason]);

It would be great if you'd like to tackle this!

mraleph commented 2 weeks ago

Is this stale now or do we still want to land #1295?

kpsroka commented 2 weeks ago

@mraleph Hi, Sorry for dropping this. In the end, since in our (superlist's) app we'd still want to support web, we decided not to use the additional codes available, and use the custom ones. While I believe that this issue has merit outside of our usage, I won't work on this in any foreseeable future. Please close the Issue/PR as you see fit.