-
Notifications
You must be signed in to change notification settings - Fork 605
add an experimental websockets api between client and server #3463
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
Conversation
PR HealthBreaking changes ✔️
This check can be disabled by tagging the PR with Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with Coverage ✔️
This check for test coverage is informational (issues shown here will not fail the PR). This check can be disabled by tagging the PR with API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
This check can be disabled by tagging the PR with License Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
This check can be disabled by tagging the PR with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, a few minor comments
pkgs/dartpad_ui/lib/model/model.dart
Outdated
if (useWebsockets) { | ||
// Since using websockets is a compile-time config option, and it's only | ||
// used for the version call, we create it here on demand. | ||
webSocketServices ??= await WebsocketServicesClient.connect( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably still prefer to create this object in the AppServices constructor (or not, depending on the useWebsockets flag) so that this code is easier to migrate in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, sounds good - creating a websocket connection eagerly, but only if we have the compile time useWebsockets
const set.
/ws
endpointversion
command over this channelNote that the websocket connection is long-lived (we'd need to adjust the backend configuration, but we can set requests timeouts to up to 60m). Before actually enabling this, we'd want to:
Contribution guidelines:
dart format
.Many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.
Note: The Dart team is trialing Gemini Code Assist. Don't take its comments as final Dart team feedback. Use the suggestions if they're helpful; otherwise, wait for a human reviewer.