config option to change the extension

[odedniv]

In previous versions the filename remained the same across backups/reduces/resizes.

In commit f9eed2f the code was changed to change to the file type's extension, breaking my app (when I updated). I don't expect the filenames to change (even if they are wrong).

Made it optional with a config option changeExtension, that defaults to false (the original behavior). I know it's back and forth on 'breaking changes' but hopefully there are more people who haven't yet updated than there are people who have (or had to revert to the old code after finding it breaks their app).

[ysugimoto]

This is good point. But if changeExtension is false, the image converted by ImageMagick causes MIMEType mismatch and image has broken, isn't it?

To keep consistency, I think you should also stop to convert image if changeExtension is false.

[ysugimoto]

Should use Boolean(changeExtension)

[odedniv]

Will change to Boolean(), waiting for the tests to finish because I think there's another failing test and I can't run it on my machine for some reason.

How will it bother ImageMagick? He gets the raw data (see

aws-lambda-image/lib/ImageResizer.js

Line 34 in 9924bc1

let img = gm(image.data).geometry(this.options.size.toString());

) he doesn't know what filename you'll save it to. All that really matters with image types is the mime type set in S3 metadata, which this project does properly. Nobody looks at the filename, only the content type received from the HTTP web server (which S3 chooses according to the metadata).

[ysugimoto]

Yes, ImageMagick doesn't have to know. ImageData.js detects MIMEType from raw data using image-type package. If we do reduce after resize, Is there the resized image's raw data like JPEG format, but raw data is PNG? I concern about this.

If this is clearly, let's merge it.

[odedniv]

It should be fine, the type of the data (chosen by ImageMagick and determined by image-type) is completely unrelated to the output filename chosen, which can be anything and doesn't affect the data.

A browser downloading the image from S3 and showing it in an <img> tag would not care about the filename (URL) either, it only looks at the Content-Type header of the response which is sent by S3 according to the mime data set on the file, S3 doesn't care about the filename either.

The only problem is if someone takes the S3 file and determines the file type according to the extension (which is a bad way to determine), in this case the package's user can use the changeExtension option.

Can you check why travis is stuck? I think there might be one failing test I need to fix before this is merged.

[kdybicz]

Changing you app to make it expect change of the extension would seem to be more reasonable approach for me :)

[odedniv]

@kdybicz that would mean the lambda needs to update the DB (Mongo in my case). I actually have all images uploaded as "image.jpg" (to different directories) and I prefer it not to change ever 😄

[odedniv]

@ysugimoto I've been wrapping my brain around these failures and I have no clue 😢

I can barely run the test suite, I have to disable eslint and then I get a lot of Error: write EPIPE. Would also be nice to be able to run a single test. Any ideas?

[ysugimoto]

@odedniv I saw Travis's log, some assertions are failed. And eslint error reports in your environments? An error of write EPIPE might be a process I/O error in node.js, and I'm not sure why, or when that error occurs 😢

Anyway, you can run a single test to install ava globally, I think:

$ npm install ava -g
$ ava test/e2e-jpeg.js

[ysugimoto]

If we passed the tests, let's merge this

[odedniv]

This is the output I get from running the tests:

oded@plex ~/devel/aws-lambda-image $ ava test/e2e-jpeg.js 
Reducing to: in-place
Reducing to: some
Backing up to: in-place
Resizing to: in-place
Resizing to: in-place
Resizing to: in-place
Resizing to: in-place
Resizing to: in-place
Reducing to: in-place
Reducing to: in-place
Reducing to: in-place
Reducing to: in-place
Reducing to: in-place

  1 passed
  3 exceptions

  Uncaught Exception
  Error: write EPIPE


  Uncaught Exception
  Error: write EPIPE


  ✖ Test results were not received from test/e2e-jpeg.js

It doesn't make sense on many levels... for example the file contains more than 4 tests

[ysugimoto]

Hmmmmm...
I just run test as same as you, success it.

So, I'm using node v6.9.1 on macOS. Please let me know about your environment?
In addition, please reinstall ImageMagick if you can.

[odedniv]

Didn't work... Well if it succeeds for you I guess it can be merged :)