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

Increase flexibility for HTML5 uploads #1012

Closed
wants to merge 1 commit into from

Conversation

jpbochi
Copy link

@jpbochi jpbochi commented Jan 23, 2014

  • method may be specified. POST is the default.
  • URL for upload may vary according to the file. Just pass a function to url to do so.
  • Let XHR decide the content-type instead of hard-coding application/octet-stream.
  • tests + code clean up.

@jayarjo
Copy link
Contributor

jayarjo commented Jan 24, 2014

Can you describe the case when having url option as a function might be useful?

@jpbochi
Copy link
Author

jpbochi commented Jan 24, 2014

@jayarjo I working on an upload widget for Rackspace Files.

Initially, I tried to do a regular POST request to their API. However, a successful upload always replies with a redirect, and it would be a preflighted CORS request, which is not supported by design. See documentation at w3.org and MDN.

The alternative is to make a PUT call to a TempURL. Plupload only supports POST call, so I had to make an option to change that. Also, the URL is different for each file. Accepting a function for the url seemed like the cleanest way to achieve that.

@jpbochi
Copy link
Author

jpbochi commented Jan 24, 2014

BTW, I was trying to make plupload work wit flash (which could be an alternative for my case), and I couldn't.

I tried to simply modify this fiddle (linked in https://github.com/jayarjo/plupload-demos) to have only flash. Unfortunately, I get an Error #-500: Init error.. I guess there's a bug to be fixed there.

@jayarjo
Copy link
Contributor

jayarjo commented Jan 24, 2014

Is this a requirement by Rackspace that each url should be different for every file?

There is some problem with the version of the flash shim on github, simply try the one from the package.

@jpbochi
Copy link
Author

jpbochi commented Jan 24, 2014

Yes, it's a requirement of the Rackspace API. The url to make the PUT contains the file name in an encrypted pre-authenticated way. If the I want to upload a different file, a new URL is needed.

I'll try the flash from the package.

BTW, if I didn't want html4, could I use only the plupload.min.js instead of plupload.full.min.js? I tried it, but I fails to load because mOxie is not defined.

@jayarjo
Copy link
Contributor

jayarjo commented Jan 24, 2014

mOxie has to be custom compiled to include only specified runtimes and included before plupload.min.js. You only need to compile JS files, available Flash and Silverlight binaries will work just fine. And maybe you do not need to include image manipulation logic.

@jayarjo
Copy link
Contributor

jayarjo commented Jan 24, 2014

Can you provide examples of typical Rackspace URLs?

In general it should be possible to simply change url option through uploader.setOption('url', newUrl). Possibility to set HTTP method however is nice, but only possible in HTML5 - Flash and Silverlight restrict it to GET and POST only.

@jpbochi
Copy link
Author

jpbochi commented Jan 24, 2014

This is a typical Rackspace TempURL:

https://storage101.dfw1.clouddrive.com/v1/MossoCloudFS_xxx_guid_xxx/A_Container/ScreenShot_2014-01-24.png?temp_url_sig=an_hexadecimal_encoded_signature&temp_url_expires=1390585262

@jpbochi
Copy link
Author

jpbochi commented Jan 24, 2014

Flash and Silverlight could potentially use X-HTTP-Method-Override to make a POST that is actually a PUT

And I'm not sure uploader.setOption would work when uploading several files when the url has to be different for each one. One would have to synchronise the setting of the option between each upload. Also, it would make it almost impossible to implement concurrent file uploads.

Sadly, for our case, I'm starting to feel that we'll have to build a component ourselves.

@jayarjo
Copy link
Contributor

jayarjo commented Jan 25, 2014

Currently concurrent uploads are not supported. But if they were I do not see how setting url via uploader.setOption can be a problem. It can be set on BeforeUpload which is invoked (as well as UploadFile) for each file separately.

Is X-HTTP-Method-Override supported by Rackspace? S3 doesn't support it for example.

@jpbochi
Copy link
Author

jpbochi commented Jan 25, 2014

Setting the url on BeforeUpload could work just fine. I quite a fan of accepting functions when a value could vary. Underscore does it a lot. See how much this lookupIterator is used there.

Rackspace currently doesn't accept X-HTTP-Method-Override, but they re considering it. I have some contacts there. ㊙️ 😉

@jayarjo
Copy link
Contributor

jayarjo commented Jan 25, 2014

I'm not against such pattern in general, it's nice. It's just that it may lead to confusion in this particular case.

Amazon is considering X-HTTP-Method-Override for couple of years already... :) And btw, you can easily add custom headers via headers option, so there should be nothing standing in the way, if they were supporting it. In the case of Flash however the drawback is that its default upload method doesn't let you to send custom HTTP headers, so you would need to use our custom one, which requires the file to be loaded in memory first (so obviously not usable in the case of files larger than several hundreds of megabytes).

@jpbochi
Copy link
Author

jpbochi commented Jan 25, 2014

In my particular case, large file uploads are a common usage. Now that I know that flash loads the whole file into memoty, I guess it won't be an option.

@wuservices
Copy link

Thought I'd chip in that some way to have a function for an uploaded filename could be nice for me too, but if I did that the same facility would be needed for setting multipart params. I basically make an AJAX request to get a signed policy to upload a file or group of files and I was going to have to do this for each file, but decided to make a policy for some-path/* that allows uploading files with a certain prefix but doesn't limit the number of files somebody could upload with the policy (which could be a potential issue if a malicious user wanted to fill up a bucket). This is for Google Storage but I'm guessing other providers might allow wildcard prefixes too.

@jayarjo
Copy link
Contributor

jayarjo commented Jan 27, 2014

@wuservices can't you do it with the approach that I described above - through BeforeUpload? One could even return false from the listener to delay the upload, acquire all required information, alter the options and then manually trigger UploadFile event on the same file (e.g. uploader.trigger('UploadFile', file), where file is the same file object that he received in BeforeUpload handler as a second argument).

@wuservices
Copy link

@jayarjo, ah yes. Sorry I only saw a few messages in the thread. It sounds like that would work. The last part where you delay the upload is what I didn't know I could do before. Since getting a new policy via AJAX would be async, I'd need to go that method but yes that would address the issue. I've stuck with the wildcard prefix for now though to maximize performance with the risk of the user being able to upload a lot of files before the policy expires.

@walter
Copy link
Contributor

walter commented May 12, 2015

This overlaps with #1208 which is based on more recent code (merges without conflicts) and is more idiomatic of plupload, but doesn't include content-type overriding which this has.

I would add that PUT support is handy for using Amazon S3's presigned_url functionality to get temporary URLs for upload much like use case for Rackspace that this PR covers.

@jayarjo jayarjo added this to the PUT method milestone May 25, 2015
jayarjo added a commit that referenced this pull request Aug 6, 2015
jayarjo pushed a commit that referenced this pull request Aug 7, 2015
@jayarjo jayarjo closed this Aug 7, 2015
jayarjo pushed a commit that referenced this pull request Nov 30, 2016
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

Successfully merging this pull request may close these issues.

None yet

4 participants