Should Dial return a net.Conn?

[mjohnson9]

cc @shurcooL

Pending release of gopherjs/gopherjs#89,

Should Dial return a net.Conn?

I believe the idiomatic Go way would be to follow the guidelines of net.TCPConn (& co.) in having a WebSocket(Conn) that is exported and fulfills net.Conn, but have Dial return a net.Conn and allow type-assertion if the consumer needs the low-level functionality of the WebSocket(Conn).

This refactor would, more than likely, break API compatibility.

Thoughts?

[dmitshur]

I think that's the way to go. It should be done on a branch for now, of course.

[extemporalgenome]

If we're talking about net.Dial, I suggest that we only support "ws" as the network type in browsers. Node and other targets could presumably support "tcp" and the other standard targets, but it would hurt code portability (and be potentially very confusing to debug) if net.Dial("tcp", addr) spoke websocket on the wire.

[mjohnson9]

@extemporalgenome

We're talking about websocket.Dial. Is there a way to register a new net type with net.Dial?

[mjohnson9]

Working on this in the dial-net-conn-again branch.

[mjohnson9]

Sorry, I've been delayed on this. I'm unassigning myself for now. Feel free to take over; I didn't make any significant progress.

[dmitshur]

No problem. This is not a high priority since it doesn't change functionality (much or at all). One of us will get to it when the time is right and/or we're implementing something else.

[dmitshur]

One observation I've made is that the websocket.Dial API differs slightly between this package and golang.org/x/net/websocket. See, e.g., here.

Perhaps the two APIs can be made to match. Or perhaps not if it's out of scope for this library. This is just an observation so far.

[dmitshur]

Is it correct that the desktop websocket needs the origin to be supplied since it can be controlled, but in the browser, the browser sets it automatically and it cannot be overridden? If so, then it may not be possible to make the two APIs match.

[mjohnson9]

That's correct. From within the browser, it isn't possible to set the origin. However, the desired protocol can be specified. I think you're right about making the two APIs match as closely as is reasonable.

[dmitshur]

It's convenient for me to implement this now, so I'll take this.

[dmitshur]

One question is what to do about the Conn.WriteString method of *websocket.Conn. It's the only one not exposed by net.Conn.

One idea is to document Dial to say returned net.Conn implements the following interface (I can't seem to find an existing interface in std lib for this):

interface {
    WriteString(s string) (n int, err error)
}

Another is to have a websocket.Conn interface defined as:

type Conn interface {
    net.Conn
    WriteString(s string) (n int, err error)
}

Another idea is to simply drop it.

[mjohnson9]

I believe we should maintain the ability to define whether the frame should be a string or binary frame. However, there may well be a better way to do that.

[dmitshur]

I'm tempted to drop it. Its implementation is trivial:

// WriteString writes the contents of s to the WebSocket using a text frame
// opcode.
func (c *conn) WriteString(s string) (n int, err error) {
    err = c.Send(s)
    if err != nil {
        return
    }
    n = len(s)
    return
}

It basically calls the WebSocket.Send method with a string.

It doesn't seem to implement any (exported) standard library interface, and sending textual data via a net.Conn seems odd.

My argument is this. If you care about text frames, you can just use Send instead. If you care about using net.Conn, you'll likely treat it as a normal net.Conn which deals with streams of bytes. So WriteString doesn't seem to provide any value. (In fact, it might be misleading; in other scenarios, sending a []byte or string does not have such drastic difference.)

However, even if we drop it, we'd still need to consider possibly exposing Send, maybe?

I know dropping it is a slightly breaking API change, but it's unlikely anyone depends on it; this library is younger and I'd prefer to optimize its API to be better while we have a chance.

[dmitshur]

I've made PR #16 that implements my proposed plan above, please review.