dominikh / go-js-dom

MIT License
252 stars 42 forks source link

Consider adding a Bytes []byte method to File (or documenting an example of how it can be achieved). #54

Open dmitshur opened 6 years ago

dmitshur commented 6 years ago

Currently, we have:

https://github.com/dominikh/go-js-dom/blob/662b7b8f3412aba33480fc334a39340f90aafdb2/dom.go#L2460-L2465

It might be helpful and worth considering changing it to have a Bytes() []byte method:

// File represents files as can be obtained from file choosers or drag
// and drop.
//
// Reference: https://developer.mozilla.org/en-US/docs/Web/API/File.
type File struct { 
    *js.Object 
}

// Bytes reads the file f and returns its contents as a []byte.
func (f *File) Bytes() []byte {
    b := make(chan []byte)
    fr := js.Global.Get("FileReader").New()
    fr.Set("onload", func() {
        b <- js.Global.Get("Uint8Array").New(fr.Get("result")).Interface().([]byte)
    })
    fr.Call("readAsArrayBuffer", f.Object)
    return <-b
}

(Extracted from gopherjs/gopherjs#776 and #32. /cc @Inkeliz)

dmitshur commented 6 years ago

Having typed this up and considering it more closely, I no longer think this is a good fit and therefore shouldn't be done. But I posted it anyway for posterity and potential discussion.

First, the dom package tries to be a wrapper around APIs. The File interface can be used and interacted with in many ways. Using a FileReader is one of those ways, so picking it in Bytes is pretty opinionated.

Second, I don't think that code is completely correct. According to https://developer.mozilla.org/en-US/docs/Web/API/FileReader, the load event is triggered each time the reading operation is successfully completed. There's a loadend event which is triggered each time the reading operation is completed (either in success or failure). If I understand correctly, reading a large file can trigger multiple load events with partial results and finally a loadend event. Edit: I misread. It seems load is the same as loadend, except the former happens only on successful end of reading, while the latter happens on both successful and unsuccessful end of reading.

If that's the case, it might make more sense to map this to an io.Reader interface, to allow callers to "stream" a large file rather than blocking on Bytes to finish.

Third, the Bytes method would have a signature similar to bytes.Buffer.Bytes but isn't as fast, since it requires doing the equivalent of ioutil.ReadAll on an io.Reader.

One possible pivot from this is to document the approach of converting the contents of a File into []byte by documenting an example.

dominikh commented 6 years ago

I'll explain why File is exactly as it is: working with dom.File belongs into one or more separate packages that accept dom.File as input and do something useful with it. Be it exposing a full-fledged FileReader, or some helpers, or whatever other ways there are for working with files in JavaScript. File is only part of js/dom because some DOM-related APIs return these objects. I do not consider the types used for working on files to be part of the DOM API, however.

I'd prefer if we didn't document any ways of using File objects, either. Maybe we should make it clearer that File values are intended to be passed off to other packages.

dmitshur commented 6 years ago

Agreed, the code doesn't belong in the dom package.

If there's another package that implements this File -> []byte functionality, I think pointing to it would be very helpful to dom users. If there isn't, I don't think having an example would be that bad. But if you don't want it here, I'm okay with that too. We can just point people to this issue if it comes up again.

dominikh commented 6 years ago

The issue with examples is two-fold.

  1. If there is a third party package doing this, we'd indirectly be endorsing its use. I'm not willing to make that commitment.
  2. If we explain the manual way of converting File to []byte, it may easily trick people into believing that this is the canonical way of doing things, which in turn leads to issues such as this one being filed.

As far as concrete actions ago, I (well, you) would add another paragraph to the existing documentation explaining that making use of File is deferred to other packages.

You're of course also free to write said package, in which case issue 1 may not necessarily apply.

dmitshur commented 6 years ago

it may easily trick people into believing that this is the canonical way of doing things

I see the danger of that, but I still think that having an example for something commonly wanted but non-trivial is a net positive, and the downsides can be reduced by carefully wording it. E.g., "One of the ways this can be done is by using a FileReader, for example ..." is much better wording than "This is done like this ...".

But yeah, it's extra work and scope maintaining the example to make sure it provides a good recommendation rather than a suboptimal, outdated one. This work is reduced by not having an example, which isn't unreasonable. :)