Add events to file-queue helper, set up TypeScript, drop ember-cli-addon-docs, drop support for Ember < 3.28

[gossi]

Based on discussion in #316 - here is what I did:

Modernizations:

• Use ember-cli-typescript (not entirely, but for the key parts)
• Dropped Ember.A() (on those files I touched with TS) in favor of @tracked
• Use @ember/destroyables (depending on support matrix, this requires to ship the polyfill)
• I think this also prepares for addon format v2

Changes:

• Mainly the {{file-queue}} helper, which can be beautifully used like so:

{{#let (file-queue fileAdded=this.addFile fileRemoved=this.removeFile) as |queue|}}
  {{#each queue.files as |file|}}
    <span data-test-file>
    {{file.name}}
    </span>
    <button
      type="button"
      data-test-remove
      {{on "click" (fn queue.remove file)}}
    >
      x
    </button>
  {{/each}}

  <label>
    <input type="file" {{queue.selectFile filter=this.filter filesSelected=this.selectFiles}} hidden>
    Select File
  </label>
{{/let}}

• I kept original API of <FileUpload> component and used new helper under the hood, rewired appropriately
• FileQueueService can now work with DEFAULT_QUEUE name (instead of super-global queue)

Cutbacks:

• With usage of TS I had to disable ember-cli-addon-docs (maybe it is time to move, ie. field-guide or docfy?)
• Had to drop @onSelect (couldn't rewire to the new API - I don't see this as an issue, the API was never public)

Questions:

• I think, a lot of things can be marked deprecated for v5 and removed in v6
• I left quite some @TODO in the code, some things I marked as removal/deprecation candidates or questions I have

What I didn't do (yet):

• No API for the upload process, ie. (file-queue uploadStarted=... upload=...) or Queue.upload() - where does upload=() need to be attached? To (file-queue) or Queue?
• I haven't touch drag'n'drop at all. I event kept Queue._addFiles() - which is sooo ready to go 😅

-> ALL THAT by keeping the TestSuite green - overall was pretty happy to manage backwards compatibility (that went way better than I thought !!). I also checked with our app, by linking this and let the app testsuite run, green as well.

[gilest]

This is a great contribution – thank you @gossi ! Hugely appreciate you wading in here.

TS and docs

Use ember-cli-typescript (not entirely, but for the key parts)

Appreciate that TS is very nice and helpful – and I don't want to discourage you – but it's a bit presumptuous to include without at least discussing first. Would also prefer to make this change in a single PR rather than bundle it with an API update.

With usage of TS I had to disable ember-cli-addon-docs (maybe it is time to move, ie. field-guide or docfy?)

Are you volunteering to take care of this? I don't want to release without a docsite and the current one hosts versioned docs back to 2.4.4 which we'd have to archive somehow.

General responses

Use @ember/destroyables (depending on support matrix, this requires to ship the polyfill)

Yes we'll need ember-destroyable-polyfill as we currently support v3.16 and above.

Had to drop @onselect (couldn't rewire to the new API - I don't see this as an issue, the API was never public)

It was recently added and documented in #578. It has been published in betas but not an official release.

Looks like with this api filter takes care of filtering and filesSelected is available as a callback for file selection. Seems good 👍🏻 Someone will need to update the docs.

Component API

I think, a lot of things can be marked deprecated for v5 and removed in v6

True. I would be happy to more aggressively update <FileUpload>. At least by removing the attribute arguments and using ...attributes. This would be a very easy upgrade to document for users.

I guess regardless I would recommend addon consumers use the queue.selectFile method over <FileUpload> anyway... The main outstanding issue we hear about is with DOM structure and inaccessibility of hidden file input.

No API for the upload process, ie. (file-queue uploadStarted=... upload=...) or Queue.upload() - where does upload=() need to be attached? To (file-queue) or Queue?

Adding upload callbacks to the file-queue helper makes sense to me. This would need to be registered on the Queue instance like the other callbacks.

I haven't touch drag'n'drop at all. I event kept Queue._addFiles() - which is sooo ready to go 😅

This can mostly be handled internally since the API changes are happening at the queue level, yeah?

We will need to at least allow @queue to be passed in as an arg though. See details in next section.

Full API Example

For the sake of clarity I'll try post an example of using every component with this new API.

{{#let (file-queue fileAdded=this.addFile fileRemoved=this.removeFile) as |queue|}}
  {{#each queue.files as |file|}}
    <span data-test-file>
      {{file.name}}
    </span>
    <button type='button' data-test-remove {{on 'click' (fn queue.remove file)}}>
      x
    </button>
  {{/each}}

  <label>
    <input
      type='file'
      {{queue.selectFile filter=this.filter filesSelected=this.selectFiles}}
      hidden
    />
    Select File
  </label>

  <FileUpload
    @queue={{queue}}
    @filter={{this.filter}}
    @filesSelected={{this.selectFile}}
    {{!-- deprecated --}}
    @onSelect={{this.filterSelectedFiles}}
    @onFileAdd={{this.performUpload}}
  >
    <a>choose files</a>
  </FileUpload>

  <FileDropzone
    {{!-- we probably need to add --}}
    @queue={{queue}}
    @filter={{this.filter}}
    @filesSelected={{this.selectFile}}
    {{!-- deprecated --}}
    @onDrop={{this.filesDropped}}  {{!-- and many more drag and drop callbacks --}}
    @onFileAdd={{this.performUpload}}
  />
{{/let}}

I think we should make the naming consistent and replace with deprecation all onEvent callbacks with event.

Performing an upload

The currently documented way to integrate is for the consumer to pass their own callback to onFileAdd on one of the components and then call file.upload() within it.

This much nicer API means we can recommend users instead attach the same callback to fileAdded on the file-queue helper.

Final thoughts

How can I help with this?

• perform at least the minimum update to <FileDropzone>
• discuss or clarify something - happy to go real-time on Discord or a call
• make code/test/documentation contributions. Much of the docs needs updating to support this
• (not ready yet but) write an Upgrading to 5.0 doc

[gossi]

Thanks @gilest for your feedback. Let me address some of those.

Appreciate that TS is very nice and helpful – and I don't want to discourage you – but it's a bit presumptuous to include without at least discussing first. Would also prefer to make this change in a single PR rather than bundle it with an API update.

Yeah, I tried to use JS in the first place. But was too often distracted by fields that were "magically" inserted and I needed clarity to access them. So I pulled in TS. I saw discussion about using TS for this addon in the issues. I also thought about doing that in isolation as a "foundation" PR at first. But this PR would be to only install ember-cli-typescript and with subsequent PRs type one file after the other (or you lower the type-checks close to zero, when renaming existing .js to .ts files) - I've tried that as well and can tell now, this is not a good thing to do :/

With usage of TS I had to disable ember-cli-addon-docs (maybe it is time to move, ie. field-guide or docfy?)

Are you volunteering to take care of this? I don't want to release without a docsite and the current one hosts versioned docs back to 2.4.4 which we'd have to archive somehow.

Yes. It's done when it's documented. I looked into other options than ec-addon-docs, namely field-guide and docfy. Given the later plays well with TS (and probably auto-generates the API for components ?), this seems to be the go to option.

Use @ember/destroyables (depending on support matrix, this requires to ship the polyfill)

Yes we'll need ember-destroyable-polyfill as we currently support v3.16 and above.

Ok, I had this in. Couldn't find the minimum supported ember-version and because the ember-source dependency was latest version, I removed it 🤦. Haven't checked ember-try config. Gonna bring it back 👍

Had to drop @onselect (couldn't rewire to the new API - I don't see this as an issue, the API was never public)

It was recently added and documented in #578. It has been published in betas but not an official release.

Looks like with this api filter takes care of filtering and filesSelected is available as a callback for file selection. Seems good 👍🏻 Someone will need to update the docs.

Ah, I wasn't precise. It was never public as stable only tagged as beta, which according to semver is "here is a playground for a new API, we need feedback - for the time please don't rely on this". So, it wasn't released with final API guarantees yet - that's my point.

I think, a lot of things can be marked deprecated for v5 and removed in v6

True. I would be happy to more aggressively update <FileUpload>. At least by removing the attribute arguments and using ...attributes. This would be a very easy upgrade to document for users.

I'm sorry to disappoint you but ...attributes is already in use for this component and you cannot have two of them. That's why:

I guess regardless I would recommend addon consumers use the queue.selectFile method over <FileUpload> anyway... The main outstanding issue we hear about is with DOM structure and inaccessibility of hidden file input.

No API for the upload process, ie. (file-queue uploadStarted=... upload=...) or Queue.upload() - where does upload=() need to be attached? To (file-queue) or Queue?

Adding upload callbacks to the file-queue helper makes sense to me. This would need to be registered on the Queue instance like the other callbacks.

Perhaps not needed at all. Since upload() is happening in userland and filesSelected() is notifying. This solved the problem I was having. Fine to move that into the waiting hall, less worries 😄

I haven't touch drag'n'drop at all. I event kept Queue._addFiles() - which is sooo ready to go 😅

This can mostly be handled internally since the API changes are happening at the queue level, yeah?

We will need to at least allow @queue to be passed in as an arg though. See details in next section.

Yeah, I guess so. @queue would be the newest addition, while @name should receive a deprecation warning.

Full API Example

For the sake of clarity I'll try post an example of using every component with this new API.

{{#let (file-queue fileAdded=this.addFile fileRemoved=this.removeFile) as |queue|}}
  {{#each queue.files as |file|}}
    <span data-test-file>
      {{file.name}}
    </span>
    <button type='button' data-test-remove {{on 'click' (fn queue.remove file)}}>
      x
    </button>
  {{/each}}

  <label>
    <input
      type='file'
      {{queue.selectFile filter=this.filter filesSelected=this.selectFiles}}
      hidden
    />
    Select File
  </label>

  <FileUpload
    @queue={{queue}}
    @filter={{this.filter}}
    @filesSelected={{this.selectFile}}
    {{!-- deprecated --}}
    @onSelect={{this.filterSelectedFiles}}
    @onFileAdd={{this.performUpload}}
  >
    <a>choose files</a>
  </FileUpload>

  <FileDropzone
    {{!-- we probably need to add --}}
    @queue={{queue}}
    @filter={{this.filter}}
    @filesSelected={{this.selectFile}}
    {{!-- deprecated --}}
    @onDrop={{this.filesDropped}}  {{!-- and many more drag and drop callbacks --}}
    @onFileAdd={{this.performUpload}}
  />
{{/let}}

I think we should make the naming consistent and replace with deprecation all onEvent callbacks with event.

👍

Performing an upload

The currently documented way to integrate is for the consumer to pass their own callback to onFileAdd on one of the components and then call file.upload() within it.

This much nicer API means we can recommend users instead attach the same callback to fileAdded on the file-queue helper.

Yes, as said above, this needs a great documentation update.

Final thoughts

How can I help with this?

What I can think of:

• Take care of drag'n'drop in a follow-up PR
• See what parts of the API can be deprecated; an issue as start and a follow-up PR
• Documentation as follow-up PR (I will help with that)
• Feel free to ping me on discord 👍

What I will do:

• Address your PR comments
• At my work integration I've seen the queue files not being updated. That's when I went into vacation, need to figure out if this is happening in addon code or userland code
• Documentation as follow-up PR

But much likely this will all begin to happen next year 🎉

[gossi]

should this be readonly?

[gilest]

Yes it shouldn't mutated.

[gossi]

Ok, fixed the bug I've been seeing, addressed your requested changes and made some cleanup.

• I put @deprecated everywhere (at least in docblocks), so in a follow-up PR this can be done properly with deprecate() imported from ember and proper error messages.
• I left some questions above in my code where I needed to make little changes to make the tests go green. However this was kinda a blind change.

These question plus how to deal with @onSelect? I'm in favor of removing it (as I kinda killed its purpose but brought in a substitue).

That would be the last thing. Apart from that, if there is nothing left from your side, this PR is already good looking and ready to go?

[gossi]

@gilest Any updates on this? Anything for me to do? Would like to see this merged and published :)

[gossi]

rebased and fixed the linting... which needs approval again (can't this run automatically?)

[ijlee2]

@gossi Would writing await selectFiles(...) and removing await waitFor(...) help with the failing test in CI?

[gilest]

I don't quite see what you're trying to do here – but even locally for me the timing of this test seems unreliable.

When a file is selected and immediately uploaded the Queue.flush method should run, but it all happens in a very short time.

Suspect glimmer is optimising for minimal renders and deciding not to update the DOM with every change here. I got it to pass more often by adjusting the timing of mirage but it still wasn't an acceptable pass rate.

Could you explain clearly what you're trying to test? You're checking rendered page state since you mention autotracking, yeah? Is there a simpler way to test this?

[gossi]

Well, I wanna test the "uploading" state and that the UI is reacting to that state change.

When doing await selectFiles() this will be skipping over the uploading state to whatever comes after. So, not awaiting this and waitFor() the element that should pick up the UI change kinda seemed the way to go - and for what it's worth, it worked for the situations I used that. I also had tests, that weren't possible this way.

I also used a waiting timer in the BE (I used polly with a delay in our app code), that didn't worked at all, the waitFor() was much more reliable than the timer. My very assumptive exaplanation is that the runloop is queuing those steps in and allows testing in that order, if they are very laboratory and an execution in this order can be guaranteed.

That said, I'm happily taking advice for this one. Maybe we can provide better test adapters? (for mirage and polly?)

[gossi]

I've used a timeout for this case here. Let's see if CI is happy with that. Also this is not a problem introduced as part of this PR, more that this PR uncovered this issue.

As a matter of that, I created an issue to discuss ideas for solving that in the future #641 and keep the scope of this PR to "make-the-CI-green"

[gossi]

Thank you @ijlee2 for the comments.

@gilest I was looping Isaac in, because I have no idea why the two tests are failing on CI. The important piece here is, that they append --launch chrome argument to it, which actually does something to it - yet we both do not really know why.

Any idea?