Client Side Image Optimization (PR #13432)

Just a draft for now

GitHub

This pull request introduces 1 alert when merging f9d770926be86bf85e8c05802777fa0eb7f73bef into 33c3bb261a72aebe4b9088a3c4e54469bd77e514 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

This pull request introduces 1 alert when merging 9c1cba8873f7ddf5395421e94bdc3cffe67a14fa into 83a6ad32ffe75ae222028feddeca169fc5be54ac - view on LGTM.com

new alerts:

  • 1 for Missing variable declaration

Can we not use the inject method for this?

Should probably remove this :slight_smile:

    if (!this.worker) {

Trivial but you should be able to return new Promise.. instead of assigning it to a variable and returning that.

    composer_media_optimization_image_enabled: "Enables client-side media optimization of uploaded image files."
    composer_media_optimization_image_resize_dimensions_threshold: "Minimum image width to trigger client-side resize"
    composer_media_optimization_image_resize_width_target: "Images with widths larger than `composer_media_optimization_image_dimensions_resize_threshold` will be resized to this width. Must be >= than `composer_media_optimization_image_dimensions_resize_threshold`."

Minor suggestion: Rather than passing debugMode in each time you call this, you could assign a local variable below like this:

  let log = settings.debug_mode ? console.log : () => {};

Then you can do log('whatever') after that and not worry about it.

Overall this is looking great. Very strong JS skills!

There are new methods to use the service in newer versions of EmberJS, but this was the one that worked with a class in 3.15 in my tests. Also the one that is documented at Overview - Services - Ember Guides

That was the first thing I tried, but the global variable scope inside a dedicated web worker was being weird. Let me try again.

This pull request introduces 1 alert when merging 48a6732a81762706fdbded05f5619482b3b27d8f into fc0da499f83d028d4fab83a8df5cbf78fd171d6c - view on LGTM.com

new alerts:

  • 1 for Superfluous trailing arguments

To be clear I’m talking about the method we often use in our codebase already:

That doesn’t work in your class?

I tried that and it didn’t work. Maybe because I’m using native class versus the old Ember Component.extend({ method?

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

Weird! If you tried that and it didn’t work we can leave for now. Eventually we’ll get it working.

I can clean it up to the newer idiom when we updated Ember!