Improve public API

[gilest]

Since @jelhan brought up significant API changes in #306 . I thought I'd post some of my "RFC" notes about the public APIs.

Interested to hear thoughts.

{{!-- Current 2.7 API --}}

{{#file-dropzone name="photos" as |dropzone queue|}}
  {{#if dropzone.active}}
    {{#if dropzone.valid}}
      Drop to upload
    {{else}}
      Invalid
    {{/if}}
  {{else if queue.files.length}}
    Uploading {{queue.files.length}} files. ({{queue.progress}}%)
  {{else}}
    <h4>Upload Images</h4>
    <p>
      {{#if dropzone.supported}}
        Drag and drop images onto this area to upload them or
      {{/if}}
      {{#file-upload name="photos"
                     accept="image/*"
                     multiple=true
                     onfileadd=(route-action "uploadImage")}}
        <a id="upload-image" tabindex=0>Add an Image.</a>
      {{/file-upload}}
    </p>
  {{/if}}
{{/file-dropzone}}

{{!--
  3.0/4.0? API RFC

  Contextual component

  * Attach all queue callbacks via the contextual/provider component
  * Rename FileUpload component to FileSelect, because FileUpload is too generic
  * FileSelect and FileDropzone _require_ a queue to be passed in, no more name attribute
  * Dropzone and FileSelect can only add to the queue, and provide callbacks for their own actions
  * Disadvantage: the most simple usage is a little more verbose
  * Advantage: maybe easier to use, less magic, more flexible
 --}}

<FileUploadQueue @name="photos" @onFileAdd={{action this.uploadImage}} as |queue|>
  <FileDropzone @queue={{queue}} @onDrop={{action this.filesSelected}} as |dropzone|>
    {{#if dropzone.active}}
      {{#if dropzone.valid}}
        Drop to upload
      {{else}}
        Invalid
      {{/if}}
    {{else if queue.files.length}}
      Uploading {{queue.files.length}} files. ({{queue.progress}}%)
    {{else}}
      <h4>Upload Images</h4>
      <p>
        {{#if dropzone.supported}}
          Drag and drop images onto this area to upload them or
        {{/if}}
        <FileSelect @queue={{queue}}
                    @for="upload-photo"
                    @accept="image/*"
                    @multiple={{true}}
                    @onSelected={{action this.filesSelected}}>
          <a tabindex="0">Add an Image.</a>
        </FileSelect>
      </p>
    {{/if}}
  </FileDropzone>
</FileUploadQueue>

{{!--
  Rename test helpers

  * Avoid having two helper functions called "upload"

  Mirage handler
  upload => uploadHander

  addon-test-support
  upload => selectFiles
  dragAndDrop
  dragEnter
  dragLeave
--}}

[jelhan]

Thanks a lot for proposing that API. I agree with the overall direction this is going.

The only point I'm not conviced yet is the <FileUploadQueue> container component. Did you consider a {{file-upload-queue}} helper instead? The API would look like:

{{#let (file-upload-queue @name="photos" @onFileAdd=(action this.uploadImage)) as |queue|}}
  <FileDropzone @queue={{queue}} @onDrop={{action this.filesSelected}} as |dropzone|>
    {{! ... }}
  </FileDropzone>
{{/let}}

With such an API it's more clear that {{file-upload-queue}} does not render something but provides access to the queue. Also it could be used inline if the queue is only needed once:

<FileSelect
  @queue={{file-upload-queue @name="photos" @onFileAdd=(action this.uploadImage)}}
/>

Sorry that I haven't had the time to reply earlier.

[gilest]

Agree with you and like your solution very much.

I kind of forgot about helpers 😅

[gilest]

Current version of working document – editing in suggestions from #58

{{#let (file-upload-queue
          name="photos"
          onFileAdd=this.uploadImage
          onFileUpload=this.trackUpload
          onFileUploaded=this.handleUploadSuccess
          onFileUploadFailed=this.handleUploadFailure
        ) as |queue|}}
  <FileDropzone @queue={{queue}} @onDrop={{this.filesSelected}} as |dropzone|>
    {{#if dropzone.active}}
      {{#if dropzone.valid}}
        Drop to upload
      {{else}}
        Invalid
      {{/if}}
    {{else if queue.files.length}}
      Uploading {{queue.files.length}} files. ({{queue.progress}}%)
    {{else}}
      <h4>Upload Images</h4>
      <p>
        {{#if dropzone.supported}}
          Drag and drop images onto this area to upload them or
        {{/if}}
        <FileSelect @queue={{queue}}
                    @for="upload-photo"
                    @accept="image/*"
                    @multiple={{true}}
                    @onSelected={{this.filesSelected}}>
          <a tabindex="0">Add an Image.</a>
        </FileSelect>
      </p>
    {{/if}}
  </FileDropzone>
{{/let}}

[jelhan]

I like the events on {{file-upload-queue}}.

@onFileAdd might be useful for validation as well if it gives the consumer control wheter the file should be added to the queue or not. This is required for #397. Maybe it's return value could be used therefore? 🤔 In Ember Bootstrap we have some APIs which allow consumers to stop an action by returning false from an action handler. We could maybe do something similar here. E.g. not adding the file to the queue if action handler passed to onFileAdd returns false.

Wondering how @onDrop action handler of <FileDropzone> and @onSelected of <FileSelect> play together with the action @onFileAdd of {{file-upload-queue}}. Not saying that they wouldn't be needed. I guess there are valid use cases for both. But seems to be important to specify the timing of them - especially if we support async functions / promises. Also we need to discuss if all of them should have the same capabilities.

<FileSelect> seems to have many arguments that map directly to HTML attributes. If that's their only concern we should support setting them with angle bracket invocation instead.

I'm not sure about @multiple argument. If all that it does is setting the HTML attribute on <input type="file">, it should be removed in favor of angle bracket invocation. If it prevents adding another file until the first one is uploaded, it should be moved to the {{queue}} instead.

I'm not sure about name argument of a {{file-upload-queue}}. I get that we should be able to use an existing queue. But what happens if a consumer calls {{file-upload-queue}} twice with different arguments? I guess it updates the configuration of the existing queue? If so: Would it remove action handlers registered already or add additional once? What happens when the last instance of {{file-upload-queue}} is removed from template? Is the queue destroyed? What if the queue still has files, which aren't uploaded yet? If it's not destroyed, what I to the action handlers attached by that {{file-upload-queue}}.

To be honest I'm not sure if all that questions are specified for the existing implementation. Not having them defined might be the reason for some of the bugs and confusion we are facing.

[gossi]

We are having some problems with the shortcomings of the current API as pointed in #572. I tried the suggested fix with recent beta, just to learn the API is actually misleading and not solving the problem we are having.
I also always felt there was some kind of twist in the API, which kept me wondering. I felt confused. I like the addon and the problem it solve and am about giving back. So I was hunting down: what is it that confuses me? I

After starring long enough at the code, trying to make sense of it, I finally got it and am here to share what that is and my suggested solution to it.

Ambiguity and Unclarity

That's actually a thing I see quite often happening to myself in my projects, especially when they grow. You start with some thing (such as an object or entity) and over time you'll add more and more functionality to it. At some point the original intent becomes blurry and when I realize this, I'll stop and start redefining things, settle on a naming or whatever is necessary to bring back clarity. Well I'm not part of this from the beginning but as an addon consumer this isn't clear to me either. Some examples where things are blurry:

• The @onFileAdd arg actually should handle the upload of a particular file
• The @onSelect arg should filter a selection

Naming is important and here things don't go well together in my opinion, which is one factor of confusion. Another one is the mix of concepts and blurring the lines of responsibilities:

• The arg added to one UI element is actually handling the whole queue
• There is a service that partially acts as a queue (at least for the super-global files)

On the other hand, the things I do like (judging from the code) is especially the File and the Queue. They are quite solid! That's good, I used them to mentally organize what I was finding the addon.

Aquisition

In order to do that, I was unconsciously using the OOUX definitions for objects, actions and relationships (+ events). I started with an inventory of things I could find relevant for the purpose of handling uploads. Not necessarily related to what's available in this addon but purposely exaggeration to probe the scope, to later reduce it is what I like to do.

Objects (nouns):

• Super-global Queue - aka the queue-like service
• Queue
• File
• UI Elements - Dropzone, File Select

Actions (active verbs):

• Select - picking a file from the file dialog
• Filter - filtering the file selection
• Add - adding (filtered) files to the queue
• Remove - removing a file from the queue
• Start Upload - triggers the upload for ... a collection of files or a queue
• Upload - sending the bytes, upload a batch (files[]) or one file (file)

Relationships:

• Super-global queue vs Queue - to be honest that relationship is difficult
• Queue 1:n File
• Queue 1:n UI Elements
• UI Element 1:n File

Events (passive verbs):

• uploaded - A file or queue has finished uploading
• upload failed - An upload has failed
• upload started - The uploadd for a file or queue has been started
• selected - User has chosen file in the file dialog and file has been approven by a possible filterFile

+ drag and drop events (as I'm totally not into this, I'll leave them out).

They all trigger a state change in the File, which is already handled pretty well :hooray:

Possibly also these events (more for completeness, but I don't consider them relevant for the addon):

• file added (to the queue)
• file removed (from the queue)

Objects and their Actions and Events

So far, the list was a flat discovery to grasp the (whole) domain. Instead when you read the descriptions one more time, they contain a reference to the object where they can appear (queue and/or file). So, let's do these assignments. I will use TS to express this (in some sort of DSL). I will also only focus on the discovery list above (and by that skip existing methods and properties on those objects already).

// actions

/* Actions for files */
interface FileActions {
  upload?: (file: File) => Promise<void>;
}

/* Actions for selection */
interface SelectionActions {
  filter?: (file: File) => boolean;
}

// events
// they take a name with an optional _payload_ (not the real structure, just to express what might be carried)

/* Events for queue(-likes) */
interface QueueEvents {
  added: File | File[];
  removed: File | File[];
  cleared: undefined;
}

/* Events for upload */
interface UploadEvents {
  started: File | File[];
  failed: File | File[];
  uploaded: undefined;
}

/* Events for selection */
interface SelectionEvents {
  selected: File | File[];
}

// objects

enum FileState {
  // ...
}

interface File<Params extends {
  Actions: FileActions;
  Events: UploadEvents;
}> {
  state: FileState;
  upload();
}

interface Queue<Params extends {
  Actions: FileActions & SelectionActions;
  Events: QueueEvents & UploadEvents;
}> {
  files: File[];

  add(fileOrFiles: File | File[]);
  remove(fileOrFiles: File | File[]);
  clear();

  upload();
}

interface UIElement<Params extends {
  Options: {
    /* The assigned queue for that particular element */
    queue: Queue;
    multiple?: boolean;
  }
  Actions: SelectionActions,
  Events: QueueEvents & UploadEvents & SelectionEvents
}> {
  /* A subset of the files from the queue associated with this UI element */
  files: File[];

  select(): File[];
}

Quite some stuff, yet grouped and distinctively organized, bringing clarity. Which object is responsible for what, what it can do and what does it "report" on. I might not nailed all cases nor would I expect this to be accurate. Usually, this is somewhat around 80% correct and you'll figure out the 20% "oupsy" while implementing ;) Even with that ~80% that's ok to move forward.

Responsibilities

Let's clear up some responsibilities:

Queue:

• Will be concerned with all the files added to the queue
• One queue is managed through one to many UI elements
• => Operates on Queue level

UI Elements:

• Is connected to a queue
• But will be only concerned with the files selected from that UI element
• That file collection can be a subset of the queue (or the queue in its entirety, if there is only one UI element for that queue)
• => Operates on File level

Solution / Suggested API

The existing API enforces quite some conventions that are unfortunately not overridable, which heavily limits the usage of this addon (and prohibits for example our use case). With the achieved clarity and cleared responsibilities, a new API can be thought of, that allows flexibility of use when needed while providing a sensible default out of the box (ok, the sensible default in this case means backwards compatibility).

For that, I really, really... really (!) like the idea of having a helper over a provider component to manage the queue. I'm happily expand on your idea here. Let's go into a verbose sneak peak:

{{#let 
  (file-queue 
    name="gallery" 
    
    {{! ++ actions ++ }}
    upload=this.upload
    filter=this.filter
    
    {{! ++ event listeners ++ }}
    started=this.handleUploadStartForQueue
  ) 
  as |queue|
}}

  Overall progress: {{queue.progress}}
  
  <button {{on "click" queue.upload}}>Start the upload!</button>
  
  All files in the queue:

  {{#each queue.files as |file|}}
    {{#if (eq file.state 'UPLOADED')}}
      :white_check_mark: this will almost never be shown (or too short to be visible) 
      as uploaded files will be removed from the queue
    {{/if}}

    {{file.name}} - Uploading at {{file.progress}}

    {{#if (eq file.state 'FAILED')}}
      oh oh, sample error message herer
    {{/if}}
  {{/each}}

  <DropZone
    @queue={{queue}}
    @multiple={{true}}
    
    {{! ++ event listeners ++ }}
    
    {{! 
      block automatic upload after files got dropped, aka "selected" }}
      collecting files first and start upload later 
      also see next example for default behavior made explicit
    }}
    
    @selected={{false}}
    as |zone|
  >
    Files in this dropzone:
    
    - inside the block through yielded files
    - with the ability to remove the from the queue
    - to realize something like a "review" behavior

    {{#each zone.files as |file|}}  
      {{file.name}} <button {{on "click" (fn queue.remove file)}}>remove file</button>
    {{/each}}
    
    <button {{on "click" queue.clear}}>remove all files</button>
  </DropZone>
  
  <FileSelect 
    @queue={{queue}}
    
    {{! ++ actions ++ }}
    
    {{! local overwrite to the handler on queue - dunno if useful at all }}
    @filter={{this.filterForSmall}}
    
    {{! ++ event listeners ++ }}
    @started={{this.associate}}
    @uploaded={{this.clear}}
   
    {{! that *is* the internal default behavior, *explicit* verbosity only for demo purposes}}
    @selected={{queue.upload}}
  >
    Choose Files
  </FileSelect>
  
  Files selected as siblings to the UI element:
  
  {{#if this.file}}
    {{#if (eq this.file.state 'UPLOADED')}}
      :white_check_mark:
    {{/if}}

    {{this.file.name}} - Uploading at {{file.progress}}

    {{#if (eq file.state 'FAILED')}}
      oh oh, sample error message herer
    {{/if}}
  {{/if}}

{{/let}}

backing class:

import { File as UploadFile, Queue } from 'ember-file-upload';

export DemoComponent extends Component {
  @tracked file?: UploadFile;
  
  @action
  associate(files: UploadFile[]) {
    this.file = files.length > 0 ? files[0] : undefined;
  }

  @action
  clear() {
    this.file = undefined;
  }

  @action
  upload(file: UploadFile) {
    // send the bytes to the backend in here
  }
  
  @action
  filter(file: File) {
    return file.type.includes('image'); // images only
  }
  
  @action
  filterForSmall(file: File) {
    return this.filterFile(file) && file.size < 500 * 1024; // max 500kb
  }
  
  @action
  handleUploadStartForQueue(queue: Queue) {
    console.log(`let's get the whole **queue** going!`, queue);
  }
}

This allows to distinctively work on queue or file/ui element level, choose whatever scope works for addon consumers.

It favors pull-based reactivity where possible and expands to push-based reactivity where pull-based

API

Here is the public API for the usage. For components I follow RFC #748:

type QueueHelperArgs = {
    name: string;
  }
  & FileActions 
  & SelectionActions;
  & QueueEvents 
  & UploadEvents
;

type QueueHelper = (positional: unknown[], named: QueueHelperArgs): Queue;

interface FileSelectSignature {
  Element: ???;
  Args: {
      queue: Queue;
      multiple?: boolean;
      // ... + more
    }
    & SelectionActions
    & QueueEvents 
    & UploadEvents 
    & SelectionEvents;
  BlockParams: {
    default: {
      files: File[];
      progress: number;
      // ... maybe more here?
    }
  }
}

Migration

You already named the new component <FileSelect> over the current <FileUpload> one. With that its free to start working with it. As <FileUpload> is coupled to a <label> element and there are a couple of ways to make accessible file uploads, I could think of <FileSelect> as an invisible component. It will render <input type="file"> and would yield a select() method to trigger this to popup (pre-wired to the element, no manual on click shenanigans). ...attributes are forwarded to the <input> directly. I think, this would improve the API one more time (ie. <FileUpload> could use <FileSelect> itself). Also this is being talked in #387. I'd more than happy to make that possible, but I'd als need to rely on experts here, as I've never ever was in the situation to make file upload accessible before.

Also, I think, the existing components can continue to work. The existing args will be marked as @deprecated (and internally alias/re-assigned) for v5 and will be dropped in v6.

Implementation

Overall, I don't think, this is much work tbh (the hard part is mostly getting everything structured). Please have a look and validate my suggestion, try to break it. If we can agree on something in week 45, I can support implementation of this in week 46.

[gilest]

Hi @gossi. Appreciate your enthusiasm!

That's actually a thing I see quite often happening to myself in my projects, especially when they grow. You start with some thing (such as an object or entity) and over time you'll add more and more functionality to it. At some point the original intent becomes blurry and when I realize this, I'll stop and start redefining things, settle on a naming or whatever is necessary to bring back clarity. Well I'm not part of this from the beginning but as an addon consumer this isn't clear to me either.

Yes – current maintainers did not design much of the API 😅. This has been through a few iterations in its past and we picked up maintenance when it was in a sorry state.

It started as a single component with args that closely mimicked html attributes for an <input type="file" > and then was migrated to supporting dropzones also. There's some inconsistent behaviour, API naming and frankly broken stuff as a result.

We've been working hard to unblock the upgrade path for users by paying tech debt and ensuring we have a released version that is compatible with Embroider/Ember v4. That has been a lot of work.

This is really the main remaining job – and from a code perspective you're right that it's not that difficult. It's the design that is hard – and you have a lot to add here!

My progress

A few design decisions I had made.

• Dropped @multiple arg – easy enough for a user to implement with their filter
• A provider component as it allows the @queue arg to be curried and makes the template a little simpler . In my implementation using the file-queue helper and passing @queue to the upload components works fine also – it's more verbose. Kind of a matter of taste this one 🤷🏻
• Make @name/name file-queue helper and FileQueue provider argument optional. They will use a default queue if a name is not provided
• Events passed to a queue (from helper or provider component) are registered and unregistered as they enter/leave the DOM. This is a long-running bug (remember I thought it's what your issue was about)
• Make a very simple modifier available for users who need to a lot of control over DOM elements (accessibility concerns)

Provider demo:

<FileQueue @upload={{this.upload}} as |queue|>
  <queue.Dropzone as |zone|>
    {{#if zone.supported}}
      {{if zone.active 'Let go to upload' 'Drop files here'}}

      <queue.Select as |select|>
        Or choose a file with queue.Select
        <select.Input accept="image/*,video/*,audio/*" multiple />
      </queue.Select>

      <label>
        Or modify a file input with queue.fileSelect
        <input
          type="file"
          accept="image/*,video/*,audio/*"
          multiple
          {{queue.fileSelect}}
        />
      </label>
    {{/if}}

    {{#if queue.files.length}}
      Uploading {{queue.files.length}} files. ({{queue.progress}}%)
    {{/if}}
  </queue.Dropzone>
</FileQueue>

Same codebase supports helper version:

{{#let (file-queue upload=this.upload) as |queue|}}
  <FileDropzone @queue={{queue}} as |zone|>
    {{#if zone.supported}}
      {{if zone.active 'Let go to upload' 'Drop files here'}}

      Drag into queue.Dropzone
      <FileSelect @queue={{queue}} as |select|>
        Or choose a file with queue.Select
        <select.Input accept="image/*,video/*,audio/*" multiple />
      </FileSelect>

      <label>
        Or modify a file input with queue.fileSelect
        <input
          type="file"
          accept="image/*,video/*,audio/*"
          multiple
          {{queue.fileSelect}}
        />
      </label>
    {{/if}}

    {{#if queue.files.length}}
      Uploading {{queue.files.length}} files. ({{queue.progress}}%)
    {{/if}}
  </FileDropzone>
{{/let}}

My blockers

• Time and motivation
• Uncertainty about event API signatures and naming
• Uncertainty about the ideal FileSelect api

As is coupled to a element and there are a couple of ways to make accessible file uploads, I could think of as an invisible component. It will render and would yield a select() method to trigger this to popup (pre-wired to the element, no manual on click shenanigans).

This is an interesting idea – worth trying. I guess as long as the select method is called from an interaction event (and the button that triggers it has a tabindex) this may work well 😄.

I feel like my modifier solution is still helpful as it makes no DOM decisions at all for users and is a future-proof "escape hatch" for emergencies.

Event naming and deprecation

I'm happy to make a bold release for v5 which has an entirely new component API. Personally I don't think it's worth maintaining the old components for another major.

I like your ideas about the events so keen to follow your lead there.

Going forward

If you'd like please make a PR. Appreciate that users are waiting for v5 - particularly for accessibility.

Would appreciate if you could seriously consider the decisions I made in "My progress".

I can assist with review, tests and docs.

Would be good to hear any guidance from @jelhan too

[gossi]

Thanks @gilest for sharing your way of coming here. I think both of ours very much align here :)

I'm pretty much on it to submit a PR to make it work. After processing your ideas, I think this is what sounds the minimal effort to help out our situation:

• Update Queue to register multiple listeners, ie. add Queue.addListener() and Queue.removeListener() methods
• Update file-queue helper to accept either fileAdded and fileRemoved parameters - or - uploadStarted one, would figure out during implementation what's the best one here
• The file-queue helper to yield a modifier {{queue.selectFile}} as I think that's the least invasive thing to do here while at the same time allows the most flexible usage (should make all accessibility problems been solved in userland - which is a good thing).

Either the helper of the modifier will take another parameter to be notified that a particular upload is started (to handle visual feedback for that particular element). Would figure out during implementation, what seems to be a good way (but I gonna use the domain model I outlined above).

Provier components are much like a thing swapping over from react land, where ember has helpers for that - so gonna use them. The only reason to use components over helpers is, when they should yield a component (which does not work from within helpers). However this addon allows a strict separation between helpers and components and their distinctive use-case, so it's fine to keep them that way.

wdyt ?

[gilest]

ember has helpers for that - so gonna use them

Fair – I think @jelhan might be of the same opinion so I'll defer to you on this one.

After processing your ideas, I think this is what sounds the minimal effort to help out our situation.

Great! I do think it's important here to implement some of the other API changes at the same time. I don't want to do a series of releases that make API changes with Upgrading instructions.

For example the "default" queue and optional name arg seem like an easy win.

I'd also question whether we should keep @multiple and @disabled as they are leftovers from when the entire addon emulated an <input type="file" />.

Anyway, I'm comfortable with you making a PR and happy to help with support.

[gilest]

@gossi How are you getting on? Anything I can help with?

[gossi]

I just started getting on it. Will work on it locally (linked). As soon as I have something "ready" I will open a draft PR.