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

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

Open
dmitshur opened this issue Mar 16, 2018 · 5 comments

Comments

@dmitshur
Copy link
Collaborator

Currently, we have:

go-js-dom/dom.go

Lines 2460 to 2465 in 662b7b8

// File represents files as can be obtained from file choosers or drag
// and drop. The dom package does not define any methods on File nor
// does it provide access to the blob or a way to read it.
type File struct {
*js.Object
}

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
Copy link
Collaborator Author

dmitshur commented Mar 16, 2018

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.

@dmitshur dmitshur changed the title Consider adding a Bytes []byte method to File. Consider adding a Bytes []byte method to File (or documenting an example of how it can be achieved). Mar 16, 2018
@dominikh
Copy link
Owner

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
Copy link
Collaborator Author

dmitshur commented Mar 16, 2018

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
Copy link
Owner

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
Copy link
Collaborator Author

dmitshur commented Mar 16, 2018

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. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants