addon rewrite

[miguelcobain]

This PR:

• updates ember/ember-cli and respective dependencies using ember-cli-update
• run ember-modules-codemod (no more Ember global)
• updates tests to conform with recent best practices (with the help of the ember-qunit-codemod)
• tests now use qunit-dom
• tests now use the click test helper
• no more jquery usage
• fixes js and hbs linting errors
• uses fastboot-transform instead of manually adding the fastboot guard

This drops support for ember versions prior to 2.5 due to the removal of ember assign polyfill. At least a minor version bump should be done.

[mminkoff]

@miguelcobain this is fantastic - thanks so much! I agree, at least a minor version bump. Actually, https://github.com/shipshapecode/ember-assign-polyfill suggests a major version bump - fine with me. I'll also add a note about compatibility to the README. Probably won't get to this until next week though.

[miguelcobain]

@mminkoff I'm planning to also PR a bump to the filestack-js version (the one we're using is pretty outdated now).

[mminkoff]

Terrific - I'll wait for that then. Thanks again for taking care of this, and so neatly.

[miguelcobain]

I updated the PR to use ember-auto-import instead of the complicated broccoli logic on index.js file.
Even better, this uses ember-auto-import dynamic import. This means that, if ember-filestack is never rendered or on fastboot, filestack javascript is never included in the build!
If it is needed, it dynamically imports the script and uses it.

Also updates the fastboot checks to conform fastboot documentation.

[mminkoff]

@miguelcobain that's fantastic! Do you still have more coming or is it ready for release?

Oh, btw, do you know if those on Ember versions prior to 2.5 can make it work by adding the ember-assign-polyfill?

[miguelcobain]

Oh, btw, do you know if those on Ember versions prior to 2.5 can make it work by adding the ember-assign-polyfill?

@mminkoff yes, if the host app adds the polyfill it should be compatible.

Do you still have more coming or is it ready for release?

since we're doing a major version bump, I'd like to propose some changes to the API. I'm writing them up in a following comment. If you endorse the proposal, I can go ahead and do it.

[miguelcobain]

1. rename ember-filestack-picker component to just filestack-picker. IMO, it's redundant to include ember- in an ember component.

2. promote all PickerOptions to top level arguments. This will allow the user to pass in the options directly to the component instead of using a single options hash argument. I think this leads to better ergonomics.
   e.g with angle bracket invocation

<FilestackPiker @accept=".pdf" @anotherOption="value"/>

instead of

<FilestackPiker @options={{hash accept=".pdf" anotherOption="value"}}/>

We would allow an options argument as well that would be merged to the other properties before calling the picker. This way the user can use whatever style they see fit or even combine both. e.g

<FilestackPiker @accept=".pdf" @options={{options}}/>
{{!--
  `options` is a hash from a controller.js file, for example.
  In this example, if `options.accept` is defined, it would be overwritten
  with `".pdf"` value, since it was explicitly passed in.
--}}

3. Global config. It is pretty common to use the same options across the entire app. The storeTo option is a good candidate for an option that would be "global". For these reasons, I think we should allow users to provide picker options in their config/environment.js file.
   We already use a filestackKey option that is always a string. Since we need to hold more options, we need an object. Also, let's not forget that the filestack init can also take some options, so this would be the perfect place to put them.
   I suggest the following format:

//config/environment.js
module.exports = function(environment) {
  var ENV = {
    //...
    'ember-filestack': {
      apiKey: '<your-filestack-key>',
      pickerOptions: {
        imageMax: [999, 999],
        imageMin: [100, 100],
        storeTo: { ... },
        // any other picker options here
      },
      clientOptions: {
        cname: 'example.com',
		sessionCache: true,
        // any other client options here
      }
  };
  //...
}

Using the addon name as the key is a common pattern in ember addons, e.g ember-bootstrap, ember-paper, etc.
The global config picker options would be merged with the <FilestackPicker> component options with the least "priority". In other words, explicit direct arguments win, then the options hash then the global config.

4. use filestack library to build processing api urls. The {{filestack-image}} helper is nice to have. At the moment, the filestack service is doing some complex logic to build the urls itself. At least newer filestack-js versions can build those urls. This has the advantages that we are always updated with filestack's method of generating these urls. Also, less code = less bugs. Let them take care of this.

To take advantage of this, we would need to allow any TransformOptions a direct argument to the {{filestack-image}} helper.
Then we would just invoke filestack's transform method with those options and url.

In my opinion, {{filestack-url}} might be a better name for the helper, since it is perfectly valid to transform other things that aren't images (well, at least as the input of the transform).

5. make <FilestackPicker> component a tagless one. At the moment the component renders a <div></div> when it doesn't need to. We can make it a tagless component.

6. Introduce a <FilestackPreview> component. Filestack has a nice feature the lets us display a preview of a file, given its handle. This is nice because the previewer supports lots of file types like images, pdf, doc, xls, maybe videos, etc.

I'm proposing a new component the would be used as

<FilestackPreview @handle={{file.handle}} />

This component would have all the PreviewOptions in the same manner that <FilestackPicker> does, except that in this case the id option would default to the current componet element id (this.elementId). This would allow you to quickly render a file previewer in place, just be rendering the <FilestackPreview> component.

7. Internal refactors. At the moment, the filestack service API is weird, IMO. The _initFilestack method sets a promise property which consumers are supposed to .then() to know when they can use the service. I suggest we use async/await to manage this asynchronous init and set and return the filestack initialized client once done. Subsequent calls wouldn't re-initialize and would instead just return the previously initialized client instance.

This was a long one. :)
Let me know what you think.

[miguelcobain]

@mminkoff I pushed all the suggestions I wrote above, hope you don't mind.
At this point this is essentially a complete rewrite of the addon. 😆

I think some of the CDN features are now obsolete since filestack doesn't use process.filestackapi.com url anymore, for example.

This is quite a lot to review. I think the easiest way would be for you to read the updated README to see what is new!

Here it is:
https://github.com/miguelcobain/ember-filestack/blob/update-dependencies/README.md

Also tagging @scudco(the second major contributor). The more eyes we get on this, the better.

Overall, I think the addon is way simpler with this, with much less code and more features.

Thanks everyone.

[mminkoff]

@miguelcobain Sorry not to have gotten back to you sooner. Overall, I love what you've done (I haven't looked at the code yet but I assume for the moment that it's as excellent as your approach). I look forward to getting to use it.

I do have some thoughts though:

1. I'd mentioned perhaps putting a note in about how to make it work in Ember versions prior to 2.5. Maybe not a big issue at this point but might as well. At the very least mention in the Readme that it requires Ember > 2.5.
2. Maybe it's too much effort but what do you think about easing the API transition with support for the old API with deprecation warnings? I realize it's already a full version bump so it's not absolutely necessary, but this forces a full API change in order to access the new features. Ultimately those changes are probably pretty simple so I can be convinced we don't need it.

Again, this really looks marvelous and I'm excited to have this be brought up to date! Great work!

[miguelcobain]

Ok, I updated some instructions about ember compatibility.
Note that we support all ember LTS versions. There isn't any good reason to not be using at least the oldest LTS.

2. Maybe it's too much effort but what do you think about easing the API transition with support for the old API with deprecation warnings? I realize it's already a full version bump so it's not absolutely necessary, but this forces a full API change in order to access the new features. Ultimately those changes are probably pretty simple so I can be convinced we don't need it.

Indeed, that would be too much effort, maybe even impossible since the filestack-js version has changed.

What do you think about writing a migration guide?

[mminkoff]

I like the update to the Readme - thanks for that.

A migration guide sounds like the right answer. I'd love to do it but I'm in the middle of a crunch that might take a few weeks. If I find some extra time I can try to make it happen but I'm reluctant to commit to doing it soon. Can you stand waiting a few weeks?

[miguelcobain]

@mminkoff I got this. :)
Today or tomorrow I should have that. It's not a lot to write compared to the updated README.

[mminkoff]

OK, if you write it then I can find time to give it a try on my own app in the next few days. I'd like to give it a test run before publishing it, ok?

[miguelcobain]

That would be awesome. 👍

[scudco]

wew that's a lot of code.

We needed to upgrade to the latest filestack-js a few months ago so we wrote a service for filestack using ember-auto-import also.

I don't really have time to offer any opinions or thoughts on this PR, but here's what we ended up with. I had meant to share it when we wrote it to see if this addon could just be EOL'd, but got plenty distracted.

config/environment.js

  let ENV = {
    // ...
    filestack: {
      apiKey: 'my-filestack-key', 
      processCdn: 'https://special-process-cdn.cloudfront.net',
      contentCdn: 'https://special-content-cdn.cloudfront.net',
    },
   // ...

app/services/filestack.js

import * as filestack from 'filestack-js';
import Service from '@ember/service';
import scrub from 'app/utils/scrub';
import { computed } from '@ember/object';
import { getOwner } from '@ember/application';
import { last } from 'lodash';

const defaultContentCdn = "https://cdn.filestackcontent.com";
const defaultProcessCdn = "https://process.filestackapi.com";

export const defaultPickerOptions = {
  accept: ['image/*'],
  maxSize: 10*1024*1024,
};

export default Service.extend({
  defaultPickerOptions,
  contentCdnRegex: computed('contentCdn', function() {
    return new RegExp(`^(${defaultContentCdn}|${this.contentCdn})`);
  }),
  processCdnRegex: computed('processCdn', function() {
    new RegExp(`^(${defaultProcessCdn}|${this.processCdn})`);
  }),
  init() {
    this._super(...arguments);

    let ENV = getOwner(this).resolveRegistration('config:environment');

    this.set('apiKey', ENV.filestack.apiKey);
    this.set('contentCdn', ENV.filestack.contentCdn || defaultContentCdn);
    this.set('processCdn', ENV.filestack.processCdn || defaultProcessCdn);
    this.set('instance', filestack.init(this.apiKey));
  },
  imageUrl(handleOrUrl, transformations={}) {
    if (!handleOrUrl) { return ''; }

    transformations = scrub(transformations);

    let handle, url, urlWithTransforms;
    let isFilestackContentUrl = handleOrUrl.match(this.contentCdnRegex);

    if (isFilestackContentUrl) {
      handle = last(handleOrUrl.split('/'));
    } else {
      url = handleOrUrl;
    }

    if (Object.keys(transformations).length) {
      urlWithTransforms = this.instance.transform(handle || url, transformations);

      if (urlWithTransforms.match(this.contentCdnRegex)) {
        return urlWithTransforms.replace(this.contentCdnRegex, this.contentCdn);
      } else if (urlWithTransforms.match(this.processCdnRegex)) {
        return urlWithTransforms.replace(this.processCdnRegex, this.processCdn);
      } else {
        return urlWithTransforms;
      }
    } else if (handle) {
      return [this.contentCdn, handle].join('/');
    } else {
      return url;
    }
  },
});

app/helpers/filestack-image.js

import Helper from '@ember/component/helper';
import { inject as service } from '@ember/service';

export default Helper.extend({
  filestack: service(),
  compute([handleOrUrl], transformations) {
    return this.filestack.imageUrl(handleOrUrl, transformations);
  }
});

then in our components when we need access to the configured filestack instance

Component Snippet

  filestack: service(),
  didReceiveAttrs() {
    this._super(...arguments);

    this.set('options', assign({}, this.filestack.defaulPickerOptions));

    filestackPickerEvents.forEach((eventName) => {
      this.set(`options.${eventName}`, this.get(eventName).bind(this));
    });
  },
  didInsertElement() {
    this._super(...arguments);

    this.set('picker', this.filestack.instance.picker(this.options));
  },

From a quick glance it seems @miguelcobain has created a much more fleshed out, addon-worthy set of changes than our purpose-built implementation.

@mminkoff Hope this is at least some help and further proof that the ember-auto-import approach has been working well for us too.

[mminkoff]

@scudco I did find all of that very helpful - thanks so much for taking the time to put it together. I'm glad to hear that ember-auto-import has worked well for this, and I agree that what @miguelcobain has done makes it, as you say, addon-worthy.

[miguelcobain]

@scudco from my investigations, filestack doesn't seem to be using process urls anymore. All files, even if they have transforms, are accessed through the normal cdn.
So I think a single CDN option should be enough, right?

[scudco]

@miguelcobain yeah you should feel free to ignore the implementation of imageUrl in the code above. That's all legacy/backwards compatibility stuff that came along for the ride when we switched to our own service.

[miguelcobain]

@mminkoff I added a migration guide: https://github.com/miguelcobain/ember-filestack/blob/update-dependencies/MIGRATION.md

Let me know if you need something else from me.

[miguelcobain]

There's actually one last thing. As the filestack-js's readme outlines, it requires a Promises implementation in order to work: https://github.com/filestack/filestack-js/blob/master/README.md#promises

That means that, if we don't add it, it would fail on older browsers.
We can add a polyfill based on the projects targets.js, but that would mean the user would end with two promises implementations: the polyfill and RSVP that is shipped with ember.

I asked on ember's discord for how to proceed, but the solution will be to fix ember-cli-promise-polyfill to polyfill ember versions correctly.

[mminkoff]

The migration guide looks great. Small thing - let's make the handleOrUrl parameter the same in the old and new versions - some folks might think that parameter name matters somehow.

What do you mean the solution will be to fix ember-cli-promise-polyfill to polypill ember versions correctly? Are you suggesting that we're dependent on a change there in order to push this comfortably, or for it to work in older versions?

[miguelcobain]

The migration guide looks great. Small thing - let's make the handleOrUrl parameter the same in the old and new versions - some folks might think that parameter name matters somehow.

Nice catch. Updated.

What do you mean the solution will be to fix ember-cli-promise-polyfill to polypill ember versions correctly? Are you suggesting that we're dependent on a change there in order to push this comfortably, or for it to work in older versions?

ember-filestack will depend on ember-cli-promise-polyfill.
ember-cli-promise-polyfill will be responsible for correctly adding a Promise polyfill only if needed. In other words, if your app's targets.js lists a browser that does not support promises, the polyfill will be added to vendor.js file.

Right now that addon is kind of in a broken state because it doesn't add the polyfill on versions below 3.4 to prevent adding multiple promise implementations.
I just need some time to think about this and fix ember-cli-promise-polyfill in a way that makes sense for all ember versions.

Meanwhile, this PR should work fine on any browser that supports Promises natively.

[mminkoff]

So do you suggest waiting for the fixed polyfill or perhaps putting a note in the README that you shouldn't update to the new version if you target older browsers?

[miguelcobain]

I suggest we wait a little bit more. :)

[mminkoff]

Works for me. I won't be able to test this out until next week.

[miguelcobain]

@mminkoff Ok, so I added the polyfill. Everything seems good to merge for me!

[mminkoff]

@miguelcobain I just went through the migration docs and it was super-easy!

One thing, though. As easy as it is to do the updates, there's no real upside (other than not doing the work) to not supporting an easier upgrade path. So I'd like to do the following before releasing this as 2.0.0:

A. Support the old filestackKey in environment.js. Since no new settings there are required this seems easy enough to do.
B. Add wrapping components for filestack-picker and filestack-url using the old names.

I've gone back and forth with myself over this, and while I have some reluctancy about it, I think it's the right thing to do.

Any objection or other thoughts?

Oh, btw, in the migration guide, you have maxSize in the "old" filestackPickerOptions but not as a param for filestack-pickerbelow. We should add that back.

I'll be happy to make the above changes if you'd like.

[miguelcobain]

Oh, btw, in the migration guide, you have maxSize in the "old" filestackPickerOptions but not as a param for filestack-pickerbelow. We should add that back.

Ok, I fixed that.

Regarding the upgrade path, my latest commit adds the following:

• an ember-filestack-picker component that deprecates upon usage and just proxies everything to filestack-picker component
• a filestack-image helper that deprecates upon usage and just proxies everything to filestack-url helper
• deprecates filestackKey if detected
• deprecates filestackProcessCDN and filestackContentCDN if detected
• deprecates the filestack service's promise property if accessed
• deprecates the filestack service's instance property if accessed

Well, I think this covers the entire migration guide, basically. :)

My suggestion is to release this as 1.1.0. After that, we remove all the deprecated features and release 2.0.0.

This conforms very nicely with ember's philosophy of "if you're running pre-major versions without deprecations, you should just be able to upgrade to the next major version without issues".

[mminkoff]

Wow - amazing! Yes, exactly right - you can upgrade to the next version if you don't get any deprecations.

One thing though - the ember-assign-polyfill docs suggest a major version bump when it's removed, which seems fair to me, and while it won't have an advance deprecation notice, it'll be easy enough to add the polyfill back if they need it. So I think this should be 2.0.0 and we'll remove this training wheels, so to speak, for 3.0.0. Did you put an "until" in the deprecation notices? We'd want those to say 3.0.0.

[miguelcobain]

@mminkoff I see. You're suggesting two major bumps. I guess that's safer. 👍
I updated until deprecation options to 3.0.0.

[mminkoff]

Fantastic - really excellent work @miguelcobain! Thanks so much for the diligence, high quality, and friendliness.

[mminkoff]

@miguelcobain I'm going to add a reference in the README to the Filestack API changelog given that we've updated filestack-js from 0.11.4 to 1.14.1 (actually I'm going to bump to 1.14.2, which was released Friday).

[miguelcobain]

You're very welcome. Glad I could help. :)

Sounds good. Once 2.0 is out I can PR the removal of deprecated features.

[mminkoff]

Terrific, though I probably wouldn't pull it until there's a reason to go to 3.0.0. I don't see any reason to be in a rush to take out the deprecated features, do you?

[miguelcobain]

I don't see any reason to be in a rush to take out the deprecated features, do you?

I don't see a reason to not do it either, I think, and we save a few bytes on people's builds (definitely not the major argument).

The addon is essentially feature complete now, so I don't foresee any major new features coming.

[mminkoff]

@miguelcobain it's now published! Note that I had to change the integration tests back to using handlebars instead of angle brackets to get the tests to work.

[miguelcobain]

@mminkoff created #11 to bring back angle bracket invocation tests.