23 / resumable.js

A JavaScript library for providing multiple simultaneous, stable, fault-tolerant and resumable/restartable uploads via the HTML5 File API.
MIT License
4.66k stars 611 forks source link

A function as target doesn't get called! #266

Open NewPlayer2 opened 8 years ago

NewPlayer2 commented 8 years ago

When I pass a function as the target parameter to the Resumable constructor, it never gets called.

Instead the script ends up sending requests to a string representation of my function.

After a quick glance at the Resumable code, it seems to me that $h.getTarget gets called only if method is set to "octet". Otherwise the target parameter is being used as is and if it's a function, it's converted to string instead of being invoked.

cpnielsen commented 8 years ago

Hi @NewPlayer2

Could you provide us with a set of sample code for how you are using resumable.js so we can debug this further?

NewPlayer2 commented 8 years ago

Thank you for a very fast reply.

Here's what I'm doing:

var getUploadTarget = function () {return 'backend.php';} var r = new Resumable({target: getUploadTarget});

I also pass chunkSize, simultaneousUploads, parameterNamespace, fileType, fileTypeErrorCallback, testChunks, withCredentials and throttleProgressCallbacks to Resumable.

Firefox tries to send request to "./function%20()%20%7B%22use%20strict%22;return%20'backend.php';%7D" instead of "./backend.php"

cpnielsen commented 8 years ago

It seems fairly odd. Are you using the latest version of resumable? The relevant lines are here: https://github.com/23/resumable.js/blob/master/resumable.js#L179-L183 and as you can see we check to see if the target is a function and then pass the params along to that.

The URL it requests seems to indicate that the target passed is a string that basically says: function () {"use strict";return 'backend.php';}

Is it possible you can give us the actual script you use, then I can test and debug locally.

NewPlayer2 commented 8 years ago

See line 752: https://github.com/23/resumable.js/blob/master/resumable.js#L752 The variable target is passed to xhr.open. Target gets assigned on line 731.

The getTarget function you referred to is getting called on line 741 inside the if block but not in the else block. I got my test script working by (without considering any consequences) quickly adding a getTarget call in the else block too.

Might I be doing something wrong or is this a bug?

If this is still unclear, I'll be happy to make a test page for you, but it'll have to wait until later.

Thank you for the help.

cpnielsen commented 8 years ago

Basically there are two methods for uploading:

We have previously made the assumption (prior to supporting a target function) that the target parameter would be the endpoint and the above method would then define whether metadata was sent as query parameters or as part of the POST body.

In your example, it will most likely work if you pass method: 'octet' as part of the options when you instantiate Resumable, ie.:

var getUploadTarget = function () {return 'backend.php';}
var r = new Resumable({
    target: getUploadTarget,
    method: 'octet'
});

At this moment, there is no way to support a target function as well as sending the parameters in the POST body. Our assumption (which may be faulty) has been that the target function is a way to create the target URL from all the parameters, hence it would be irrelevant to send them along in the POST body as well.

NewPlayer2 commented 8 years ago

OK, thanks, I understand.

I was indeed hoping to use the target function to produce the URL for the POST requests too. I needed to fix a security issue and tried to fetch a token with AJAX and manipulate the target URL between the fileAdded event and Resumable's upload() call. It's rather ugly, but something had to be done fast. :)

Changing multipart to octet will most likely require changes to my server-side code, but I'll give it a thought.

Perhaps it would be good to mention this method issue in the documentation where it says that you can pass a function to as target.

cpnielsen commented 8 years ago

The things is, we use it as-is now, it would send the parameters both in the URL and in the POST body for POST requests if target is not a function, so some kind of rewrite is necessary.

If you need to pass along some extra token, you could make use of the headers parameter, which also takes a function that returns a key-value object that inserts it into the headers of the AJAX call. It would still require some minor changes to your server-side code, but you would get around the whole target issue.

A potential change that could fix this, is to pass along the method to get the getTarget method and use that to determine whether to return the non-function URL with or without query parameters.

donut commented 8 years ago

I was running into the same issue. Luckily, my API was already setup to accept the octet method. The documentation for the target option should probably be updated to state that this must be used with method set to "octet".