peers / peerjs

Simple peer-to-peer with WebRTC.
https://peerjs.com
MIT License
12.15k stars 1.42k forks source link

`peer.connect` emits error to peer instead of to connection when no connection can be made #1281

Open louwers opened 1 month ago

louwers commented 1 month ago

Please, check for existing issues to avoid duplicates.

What happened?

When no connection can be made, this error is emitted:

https://github.com/peers/peerjs/blob/6bfc32015e5bc6df2fe1d502c071f80f2d4b278a/lib/peer.ts#L380

The docs state that:

Errors on the peer are almost always fatal and will destroy the peer

Of course, not being able to connect to another peer is definitely not fatal.

How can we reproduce the issue?

No response

What do you expected to happen?

I expected an error to be emitted to the connection as well.

I am not the only one.

If we look at this official(?) example

    connectPeer: (id: string) => new Promise<void>((resolve, reject) => {
        if (!peer) {
            reject(new Error("Peer doesn't start yet"))
            return
        }
        if (connectionMap.has(id)) {
            reject(new Error("Connection existed"))
            return
        }
        try {
            let conn = peer.connect(id, {reliable: true})
            if (!conn) {
                reject(new Error("Connection can't be established"))
            } else {
                conn.on('open', function() {
                    console.log("Connect to: " + id)
                    connectionMap.set(id, conn)
                    resolve()
                }).on('error', function(err) {
                    console.log(err)
                    reject(err)
                })
            }
        } catch (err) {
            reject(err)
        }
    }),

The Promise connectPeer returns will resolve if a peer cannot be connected to, since the error is not emitted to the connection.

Environment setup

Not really relevant.

Is this a regression?

No response

Anything else?

No response

louwers commented 1 month ago

This is not a bug but rather something unexpected.

louwers commented 1 month ago

I still think this is a bug, because there is no reliable way to extract which peer failed to connect from the error.