ProgerXP / FileDrop

Self-contained cross-browser pure JavaScript class for Drag & Drop and AJAX (multi) file upload.
filedropjs.org
The Unlicense
264 stars 61 forks source link

adding support for other HTTP methods in sendTo #6

Closed bollwyvl closed 10 years ago

bollwyvl commented 10 years ago

tl; dr: I added an options object to sendTo and sendDataReadyTo which allows for specifying a method other than POST

Thanks for the library!

my problem

I am using jquery.dav to work with files on a WebDAV server (i.e. SharePoint). I have limited visibility into the server permissions, and my files can be large, so my approach is to "fail fast":

At first, I tried to use file.readData, and put the content up myself, but was getting poor results with binary files... it is likely I was using readData incorrectly, or don't understand how to use the File API. Further, for large files, I'd like to avoid reading the data into memory unless absolutely necessary.

my hack

However, hacking the filedrop source directly to use PUT solved my problems, for both text files and binary files, so I went ahead and monkeypatched sendDataReadyTo in my code to do what I want, but would like to see this option as part of the filedrop itself, if possible.

However, having only started using filedrop, I am not sure of the right way to add this to the API, since POST is specified as part of the xhr.open method, and cannot be overridden with File.event('xhrSetup') or File.hookXHR. The easiest is probably to just add method argument to .sendTo (and therefore .sendDataReadyTo).

my solution

This PR takes a somewhat-more liberal position, and adds an options object to the key methods... it looks like only async, username and password would be used otherwise in this exact way, and those are unlikely to be very useful, but perhaps there are other possibilities I haven't thought of.

testing

I notice that sendTo does not appear in the test suite. I could support setting up a .travis.yml to add some testing against a WebDAV server (I use WsgiDav for automated testing), if that would help get this feature added.

related

a successful PUT is returning a 204 No Content, for which I am having to use

file.event("error", function(xhr){
  return [xhr.status === 204 ? success : fail](xhr);
})

Which seems a bit odd, but I am not sure how much HTTP-fu needs to be baked in... perhaps another day...

Thanks again!

ProgerXP commented 10 years ago

I see your problem. I have made a slightly different solution: I've added method to File.opt and also opt parameter for sendTo. Now to override the method you just do this:

files[0].sendTo('xxx/yyy', {method: 'PUT'})

Or, file-wise:

files[0].opt.method = 'PUT'
// ...
files[0].sendTo('xxx/yyy')

How it plays with you?

At first, I tried to use file.readData, and put the content up myself, but was getting poor results with binary files...

If you were more specific I could try helping you out. Generally this one is really simple. You can even read (and upload) files partially which is reflected in the demo on filedropjs.org.

I notice that sendTo does not appear in the test suite.

Yes, only global utility functions are tested. You are welcome to write as many tests as you want and submit them.

Which seems a bit odd, but I am not sure how much HTTP-fu needs to be baked in... perhaps another day...

I prefer leaving the error reporting up to the user of the library without overriding any default browser functionality (this at least seems to be consistent). If your use case particularly needs some other form of error handling due to different status codes - just implement that locally.

bollwyvl commented 10 years ago

@ProgerXP Thanks for the quick response.

files[0].sendTo('xxx/yyy', {method: 'PUT'})

This solution is fine with me, especially as it has the same signature as the code I'm already using.

If you were more specific I could try helping you out.

Basically, I am doing some file renaming with sjcl to ensure good, unique file names that can be verified: I wanted to be pretty confident that the data on the user's machine, in the browser, and on the server all matched. I couldn't find a way to get out data with the read* functions in a format that I could validate with what I got at the command line: Text worked fine, binary didn't work simply.

I'd look into it more, but as in this case the problem could be in any number of places, and the PUT solution works, I'll take it! It covers my use case better anyway, as in most cases the potentially large files won't even leave the native code of File API. I will check out the samples, though, thanks for the suggestion!

Yes, only global utility functions are tested. You are welcome to write as many tests as you want and submit them.

If I run into any more issues, I may well do that.

I prefer leaving the error reporting up to the user of the library

My point was just that using .event('error') seems a little odd, as a 204 No Content is not an error, per se. If I wasn't creating my file first, the PUT would return 201 File Created. I had to use .event('any') and dig around in the logs until I thought to look at my network log and read up on this verb. So basically, the 2xx responses aren't errors. For example, jQuery defines success to be where:

status >= 200 && status < 300 || status === 304

Hope this helps!

ProgerXP commented 10 years ago

Text worked fine, binary didn't work simply.

This is really strange because if you go to the demo at http://filedropjs.org/demo/#text and try dropping either binary or text files they both will show up in the text area. Just FYI.

My point was just that using .event('error') seems a little odd

It might be but I prefer just to trigger done when it's 100% "right" - code 200. This is easier to remember and more explicit if you want things behave differently so you code them. Technically I agree that 2xx are not errors according to the spec.

I understand that name "error" might not be entirely intuitive but on the other hand since the use of this handler is mostly for handling errors (or non-standard situations) renaming it to something else would obscure its purpose too much.

I have updated the docs, now it tells explicitly about the 200 code. Let's hope that this will solve the problem for everyone.