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.

Sign up for GitHub

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

Jump to bottom

Use a Uri parameter instead of a String in WebSocket.connect #49131

Open
hacker1024 opened this issue May 28, 2022 · 2 comments
Open

Use a Uri parameter instead of a String in WebSocket.connect #49131

hacker1024 opened this issue May 28, 2022 · 2 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-io P3 A lower priority bug or feature request triaged Issue has been triaged by sub team type-enhancement A request for a change that isn't a bug

Comments

@hacker1024
Copy link
Contributor

hacker1024 commented May 28, 2022 •
edited
Loading

WebSocket.connect takes the URL as a String, and then immediately parses it. If the URL already exists as a Uri before calling the function, it must be converted to a String and then parsed into a Uri again.

This is unnecessary work. Wouldn't it make more sense to take a Uri parameter? This way, the caller can just parse a string manually if need be.
@srawlins srawlins added the area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. label May 28, 2022
@hacker1024
Copy link
Contributor Author

I should mention, I'm talking about WebSocket from dart:io.

@lrhn lrhn added library-io type-enhancement A request for a change that isn't a bug labels May 29, 2022
@brianquinlan brianquinlan added P3 A lower priority bug or feature request triaged Issue has been triaged by sub team labels Dec 27, 2022
@brianquinlan
Copy link
Contributor

Changing the connect method signature would be too breaking.

Adding a new connectUri method (the naming would be consistent with HttpClient) would also be breaking (it would break code that implements WebSocket) but seems reasonable.

@lrhn lrhn added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. and removed area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. labels Apr 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-io P3 A lower priority bug or feature request triaged Issue has been triaged by sub team type-enhancement A request for a change that isn't a bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@srawlins @lrhn @brianquinlan @hacker1024