dominikh / go-js-xhr

MIT License
19 stars 7 forks source link

Feature Request: Helper like Send but for sending & receiving data in binary. #3

Closed dmitshur closed 9 years ago

dmitshur commented 9 years ago

Hi,

I think it would be helpful for this package to provide a helper similar to Send but for making an XHR that sends and receives data in binary format.

The topic of XHR and binary data is described somewhat at https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/Sending_and_Receiving_Binary_Data.

// SendBinary constructs a new Request, sends the binary data,
// and returns the response and error.
func SendBinary(method, url string, data []byte) ([]byte, error)

The motivation is twofold:

Thank you!

dominikh commented 9 years ago

Is this still required now that Send supports []byte? Do we need to set any options?

dmitshur commented 9 years ago

Now that Send supports []byte, the need/usefulness of this helper goes down, but I think it's still high enough that adding it is worth it. It would replace all my uses of NewRequest and setting ResponseType to ArrayBuffer and doing the cast from ArrayBuffer to []byte:

// My current code whenever I want to make a binary XHR:

req := xhr.NewRequest("POST", url)
req.ResponseType = xhr.ArrayBuffer

err := req.Send(data)
if err != nil {
    return nil, err
}

out := js.Global.Get("Uint8Array").New(req.Response).Interface().([]byte)
// With BinarySend it becomes:

b, err := xhr.SendBinary("POST", url, data)
if err != nil {
    return nil, err
}

What do you think?

And honestly, in my use case for Go and GopherJS, I can't think of any time I would want to send in non-binary mode, so I wouldn't miss the current xhr.Send helper if it were gone (if I need to send in non-binary, I would just do the more verbose/customizable NewRequest).

dominikh commented 9 years ago

Okay, so first it should be noted that now SendBinary isn't about sending binary (as Send with []byte does sending just fine), but about receiving a binary response. I can see the merit in that.

A quick question about the specific implementation, however: What's the type of req.Response that Uint8Array.new is required? What would be the result of req.Response.Interface()?

dmitshur commented 9 years ago

Yes, SendBinary both sends and receives response in binary. (I mentioned that in my original issue, but perhaps I didn't stress this point.)

I'll respond to your other question in a bit, gotta relocate atm.

dmitshur commented 9 years ago

Now to answer your other question (after doing some confirmation),

What's the type of req.Response ...? What would be the result of req.Response.Interface()?

Its JavaScript type is ArrayBuffer, since that type was requested via req.ResponseType = xhr.ArrayBuffer.

If you look at the table at http://godoc.org/github.com/gopherjs/gopherjs/js, there is no specific entry for ArrayBuffer in the "JavaScript type" column, hence ArrayBuffer ends up being considered as instanceof Object. Therefore, its conversion to interface{} dynamic type is map[string]interface{}.

From empirical testing, doing b := req.Response.Interface().([]byte) results in:

Uncaught Error: interface conversion: interface is map[string]interface {}, not []uint8
What's the type of req.Response that Uint8Array.new is required?

By creating an ArrayBufferView of Uint8Array kind on top of ArrayBuffer, we get the []byte internalization that we're looking for.

A few data points:

I can see the merit in that.

IMO the biggest benefit of having something so commonly needed (sending and receiving binary byte slices via XHR) here is that it will get this much scrutiny and eyes, compared to it being done in many places within people's codebases.

dominikh commented 9 years ago

At this point, I'm primarily wondering if we should be adding SendBinary, or if Send should be changed to behave like SendBinary instead. In pure Go, a string's encoding is irrelevant in data transmission, it's considered an immutable byte slice.

Is there a common scenario in which one would want to send data as text, and expect it to be transformed to be valid UTF-8 (by e.g. replacing invalid bytes with replacement runes)? If not, I'd change Send to return []byte instead of string, and to convert string data to []byte before sending it.

dominikh commented 9 years ago

See 278794072494df1b4639026075e0a21a52dcda1a for what I have in mind.

dmitshur commented 9 years ago

At this point, I'm primarily wondering if we should be adding SendBinary, or if Send should be changed to behave like SendBinary instead. In pure Go, a string's encoding is irrelevant in data transmission, it's considered an immutable byte slice.

I would prefer the latter; it would keep the API shorter and more useful.

Is there a common scenario in which one would want to send data as text, and expect it to be transformed to be valid UTF-8 (by e.g. replacing invalid bytes with replacement runes)?

I personally can't think of one at this time. Couldn't that be done outside of the context of a http request anyway? It seems like an orthogonal task.

If not, I'd change Send to return []byte instead of string, and to convert string data to []byte before sending it.

I would prefer a method that accepts and returns []byte, not string or interface{}. (Or more generally io.Reader and io.ReadCloser like http://godoc.org/net/http#Post and http://godoc.org/net/http#Response.Body (whoa, I just learned you can link to fields of a struct on godoc this way) if that could work well without performance penalties, but I don't think so.)

If you want to keep string or interface{} as a possible input type, then please have:

Send(interface{}) ([]byte, error) // Or Send(string) ([]byte, error)
SendBytes([]byte) ([]byte, error)

Or so. Similar to https://github.com/microcosm-cc/bluemonday#usage.

Unless you have good reasons that []byte is not the best input type?

dominikh commented 9 years ago

We won't return io.Reader, because the data is entirely in memory, anyway. If someone wants an io.Reader interface on top, they'll have to use bytes.NewReader

As for data being of type []byte instead of interface{}: Maybe. One can still send other types of objects (documents, blobs) and I'm not sure we should take away that functionality from the top level function. In particular, transmitting files would be dealing with Blob objects.

I think we have established that we don't want to add more functions; adding SendBytes would be no different from adding SendBinary.

dmitshur commented 9 years ago

We won't return io.Reader, because the data is entirely in memory, anyway. If someone wants an io.Reader interface on top, they'll have to use bytes.NewReader

Agreed. I was only mentioning it for reference.

As for data being of type []byte instead of interface{}: Maybe. One can still send other types of objects (documents, blobs) and I'm not sure we should take away that functionality from the top level function. In particular, transmitting files would be dealing with Blob objects.

I think we have established that we don't want to add more functions; adding SendBytes would be no different from adding SendBinary.

I propose this:

  1. Keep input type to Request.Send method as interface{} for maximum flexibility. If someone wants to send a Blob, they'll create a new request and use it.
  2. Change the higher level helper to handle the most common scenario in the best way:

    func Send(method, url string, data []byte) ([]byte, error)
dominikh commented 9 years ago

What benefit, other than slightly stricter type checking, do you gain from data []byte as opposed to data interface{}?

dmitshur commented 9 years ago

Clarity. Simpler, cleaner API.

If someone looks up the docs for xhr.Send, they'll see it can send and receive []byte, making "how do I use this func" question extremely easy to answer. If they see interface{}, they will need to spend additional mental resources (time, attention, memory) to investigate what types it really accepts, and what is the best type to feed it, should the user do conversion first, or let the library do it. All of these questions disappear if input type is constrained to []byte.

People will most likely want to send []byte, so having the slightly stricter type being enforced will not harm them at all, it will make it easier. If they want to send a string, they'll just convert it to []byte, also easy to do and little harm.

dmitshur commented 9 years ago

Closed via #6. Thank you!