-
Notifications
You must be signed in to change notification settings - Fork 128
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
add type information to the webSocketHandler
onConnection callback
#457
Labels
next-breaking-release
Issues that are worth doing but need to wait for a breaking version bump
package:shelf_web_socket
Comments
@natebosch - thoughts? |
There is not clean type system solution for this problem. Adding a signature that specifies both arguments is the right pattern to use. It is breaking, but I think we can manage a breaking version release of shelf pretty easily |
This was referenced Dec 2, 2024
devoncarew
added a commit
to dart-lang/test
that referenced
this issue
Dec 3, 2024
…od (#2421) - add a 2nd argument to the closure passed into package:shelf_web_socket's `webSocketHandler` method - widen the dep on package:shelf_web_socket This will allow us to add more type info to the closure that `webSocketHandler` expects; it's currently an untyped Function. See also dart-lang/shelf#457 and dart-lang/shelf#463. This forward declares compatibility with `3.0` of `package:shelf_web_socket`; I _think_ this is necessary - as `dart test` uses both package:test and package:shelf_web_socket - but happy to hear otherwise. --- - [x] I’ve reviewed the contributor guide and applied the relevant portions to this PR. <details> <summary>Contribution guidelines:</summary><br> - See our [contributor guide](https://github.com/dart-lang/.github/blob/main/CONTRIBUTING.md) 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](https://dart.dev/guides/language/effective-dart) and use `dart format`. - Most changes should add an entry to the changelog and may need to [rev the pubspec package version](https://github.com/dart-lang/sdk/blob/main/docs/External-Package-Maintenance.md#making-a-change). - Changes to packages require [corresponding tests](https://github.com/dart-lang/.github/blob/main/CONTRIBUTING.md#Testing). Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback. </details> --------- Co-authored-by: Jacob MacDonald <jakemac@google.com>
9 tasks
github-merge-queue bot
pushed a commit
to flutter/flutter
that referenced
this issue
Jan 6, 2025
) - update the engine and flutter_tools to be forward compatible with the upcoming shelf_web_socket v3.0 *List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.* - dart-lang/shelf#457 - dart-lang/shelf#463 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
1 task
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Labels
next-breaking-release
Issues that are worth doing but need to wait for a breaking version bump
package:shelf_web_socket
Currently, the
onConnection
callback towebSocketHandler
is untyped (it's just aFunction
).onConnection
is expecting one of two forms:or
If the first form is passed into the param then it is automatically promoted to the 2nd form
I'd like to convert the param to include type information. I think it has to be like this:
i.e., the caller has to pass in a closure that has two params, even if they never intend to use the subprotocol param. I suspect that 99% of users are currently just passing in a closure w/ one param, so this would require them to update their code.
Even making the typedef into something like
typedef ConnectionCallback = void Function(WebSocketChannel ws, [String? subprotocol]);
would require their closure to still define the optional param.Is there any typedef that would let the user not need to have a closure w/ two params?
Or should we specialize
webSocketHandler
; have the current one take a closure w/ one param (the created websocket), and have awebSocketHandlerSubProtocol()
which takes a closure w/ two params?The text was updated successfully, but these errors were encountered: