[WIP] Use uploader objects

[Subtletree]

Currently it's not trivial to customize how a file is uploaded.

A scenario:
I'm testing the new activestorage module in edge rails 5.2 for direct uploads.
It sends the client a signed s3 upload URL with a Content-MD5 header of the expected data's checksum. That data is just the file itself, not form data.

Therefore I need to override ember-file-upload to send just the file and not wrap it in a form.

This PR makes customizations like this easy by moving the uploading logic from the file model to 'uploader' objects, in the same style as ember-data's serializers/adapters or ember-simple-auth's authenticators.

3 uploaders are included by default:

• BaseUploader
• FormDataUploader (default)
• BinaryUploader

To solve the above scenario I could create an uploaders/application.js file which would be used by default:

import BaseUploader from 'ember-file-upload/uploaders/base';
export default BaseUploader;

Or pass the uploader in when uploading the file:

file.upload(url, {
  method: 'PUT',
  headers: headers
}, 'uploader:base');

Or create a custom uploader to do whatever I want:

import BaseUploader from 'ember-file-upload/uploaders/base';

export default BaseUploader.extend({
  normalizeOptions(file, url, options) {
    options = this._super(...arguments);
    // do something with options
    return options;
  },

  createData(file, url, options) {
    // customize data
    return data;
  }
});

I'm using this fork in production currently.

It needs some polishing and documenting but wanted to gauge interest in moving in this direction 🙂

These issues would be easily solved #9, #43

[tim-evans]

This is possible currently with uploadBinary

[tim-evans]

I'm not opposed to this, just want to keep the API minimal / explicit. (It does look like a nice abstraction, but I'm not sure how often this would need to be overridden)

[Subtletree]

uploadBinary is an option though it still forces the Content-Type which in my case is a signed header so s3 returns a SignatureDoesNotMatch error. It also forces a default Accepts header which needs to be whitelisted in CORS when it otherwise wouldn't need to be.

If it's that rare that more flexibility with the uploader is needed then I agree this is probably unnecessary. When rails 5.2 is released though I imagine there will be more people looking for this functionality.

Either way it was a good exercise putting it together

[tim-evans]

Ahh, cool. Could you write your requirements here, or outline what's needed? I'm super unfamiliar with uploading binary files, so it's a feature that's more community driven.

[tim-evans]

Actually, I think after sitting on this for a week, I'd be okay with this.

• Please write some tests for this ❤️
• Could you try to write to the existing API that exists (uploadBinary / upload)? Maybe these could be overridden using your solution- I don't really want to bump another major version for people because of a breaking change on a new API.
• Docs ✍️

[Subtletree]

👍 will get onto it this week

[RuslanZavacky]

@Subtletree this one is quite amazing, I think some issues that I have with uploading to S3, might be solved by this change. Do you think you can give some examples here, how to use it in the proper way (docs)? :)

Thank you!

[tim-evans]

I would hold off until this gets merged for docs, since things may change.

[Subtletree]

I've allocated some time this weekend to hopefully get this sorted.

@RuslanZavacky If you want help in the meantime feel free to ping me on the ember slack 🙂

[tim-evans]

Sorry, not the intention to close this.

[tim-evans]

Let me reopen the master branch temporarily 🙃

[Subtletree]

I've hit a bit of a road block on my last couple of attempts to get this across the line.

A file instance needs to lookup the uploader using the owner/container. The problem is that the file isn't registered so has no owner and my attempts at initializing it and adding it to the registry have failed so far.

The current code works by passing the owner down from the fileService to a queue instance to a file instance. It works fine but isn't ideal and would mean that if users of this addon wanted to manually create a file they would need to remember to pass a owner/container too.

I've tried several methods of injecting the file class into the registry in an initializer but haven't got it working so far.

I'll ask on slack if anyone with more experience in this area can take a look.

[tim-evans]

Ahh, yeah. That'd be tricky. I can try to instantiate the file using the container; that may help quite a bit

[tim-evans]

Sadly, I think it may break some of the niceness that comes from File.fromDataURL, etc.

[Alonski]

Hey @Subtletree! I have taken over maintaining this addon.
Do you feel as if this PR is still relevant? What is needed to get this over the finish line?

[Subtletree]

@Alonski Now I'm back from holiday I would like to revisit this. I think that the same thing could be accomplished much more simply by making the contentType customizable for uploadBinary, something like:

uploadBinary(url, opts) {
  // opts.contentType = 'application/octet-stream'; <-- old line
  opts.contentType = opts.contentType || 'application/octet-stream';

  return upload(this, url, opts, (request) => {
    return request.send(get(this, 'blob'));
  });
},

I think the 'uploader' idea would be useful in some situations but is mostly unnecessary. I'll have a better think about this later, whether to continue this pr or make a new one to edit uploadBinary

Good effort taking over the addon btw 🙂

[Alonski]

@Subtletree That sounds like a good idea. Test it out and if it works we can get the new PR merged in :)
Thanks!

[Alonski]

@Subtletree Would love to see this revisited. If not then this will be closed soon 🙂

[Subtletree]

I'm closing this PR and am putting aside some time to work on the simpler method mentioned above.

I'm still using this branch in production.. so I'm keen to add this functionality into master and upgrade from v2.1.1 😅