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

WebSocketService connect panic on malformed URL #726

Closed
lizhaoxian opened this issue Nov 7, 2019 · 3 comments
Closed

WebSocketService connect panic on malformed URL #726

lizhaoxian opened this issue Nov 7, 2019 · 3 comments

Comments

@lizhaoxian
Copy link
Contributor

Description

yew::services::websocket::WebSocketService::connect
this function has an unwrap

    let ws = WebSocket::new(url).unwrap();

for a malformed URL, this will result in a panic.

Expected Results

    pub fn connect<OUT: 'static>(
        &mut self,
        url: &str,
        callback: Callback<OUT>,
        notification: Callback<WebSocketStatus>,
    ) -> Result<WebSocketTask>
    where
        OUT: From<Text> + From<Binary>,
    {
        let ws = WebSocket::new(url);
        if ws.is_err() {
            return Err("Failed to create WebSocket from given URL");
        }
        ...

Actual Results

    pub fn connect<OUT: 'static>(
        &mut self,
        url: &str,
        callback: Callback<OUT>,
        notification: Callback<WebSocketStatus>,
    ) -> WebSocketTask
    where
        OUT: From<Text> + From<Binary>,
    {
        let ws = WebSocket::new(url).unwrap();

when panic happen, there is no proper way to handle it, the best solution is to have an Err returned.

Context (Environment)

  • Rust: v1.38.0
  • yew: v0.9.2
  • target: wasm32-unknown-unknown
  • cargo-web: v 0.6.26

  • browser if relevant

@jstarry
Copy link
Member

jstarry commented Nov 7, 2019

Thanks for the report, I agree we should go with your solution! I can get a fix together this weekend, if you need it sooner, feel free to send a PR

@lizhaoxian
Copy link
Contributor Author

#727

@hgzimmerman
Copy link
Member

hgzimmerman commented Nov 10, 2019

With the associated PR merged, this can be closed now.

@jstarry jstarry closed this as completed Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants