Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatic WebP support #99

Closed
YushengLi opened this issue Jun 10, 2019 · 16 comments
Closed

Automatic WebP support #99

YushengLi opened this issue Jun 10, 2019 · 16 comments

Comments

@YushengLi
Copy link

YushengLi commented Jun 10, 2019

Back when Thumbor was leveraged for image processing, we can easily configure the environment variable: AUTO_WEBP as true to make Serverless Image Handler respond with WebP images if Accept Headers sending by browsers contain image/webp.

Looks like this feature is broken when migrating from Thumbor to SharpJS, are there any plans to re-implement this feature in ThumborMapping?

Also, are there any upgrade guides or checklists available for us to follow if we want to perform the upgrade?

Last but not least, Thanks for all your hard work for putting together this amazing solution 🙇

@hayesry
Copy link
Member

hayesry commented Jun 10, 2019

Thanks @YushengLi! We're looking into the auto_webp functionality and have added it on to our backlog for further research. We've updated the implementation guide and documentation at (https://aws.amazon.com/solutions/serverless-image-handler/) with how to use the new integration with SharpJS.

Although we've built in support for most general filters via the ThumborMapping class, we still expect backward-compatibility to be an iterative process especially with more advanced features. However, we also recommend checking out the native Sharp capabilities to see if those better match your use cases. Look forward to hearing any additional feedback so we can improve the solution as needed!

@timkelty
Copy link

I added a FR on Sharp here as well: lovell/sharp#1760

@soupy1976
Copy link

I implemented this in the image-request.js class. The solution I used was something like this:

    /**
    * Checks whether the client can accept webp, returns boolean
    * @param {Object} event - The request body.
    */
    determineWhetherClientSupportsWebp(event) {
        if (event.headers.Accept && event.headers.Accept.includes("image/webp")) {
            return true;
        }
        return false;
    }

and unit tests:

// ----------------------------------------------------------------------------
// determineWhetherClientSupportsWebp()
// ----------------------------------------------------------------------------
describe('determineWhetherClientSupportsWebp()', function () {
    describe('001/AcceptsHeaderIncludesWebp', function () {
        it(`Should pass if it returns true for an accepts header which includes webp`, function () {
            // Arrange
            const event = {
                headers: {
                    Accept: "text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3"
                }
            }
            // Act
            const imageRequest = new ImageRequest();
            var result = imageRequest.determineWhetherClientSupportsWebp(event);
            // Assert
            assert.deepEqual(result, true);
        });
    });
    describe('002/AcceptsHeaderDoesNotIncludeWebp', function () {
        it(`Should pass if it returns false for an accepts header which does not include webp`, function () {
            // Arrange
            const event = {
                headers: {
                    Accept: "text/html,application/xhtml+xml,application/xml;q=0.9,image/apng,*/*;q=0.8,application/signed-exchange;v=b3"
                }
            }
            // Act
            const imageRequest = new ImageRequest();
            var result = imageRequest.determineWhetherClientSupportsWebp(event);
            // Assert
            assert.deepEqual(result, false);
        });
    });
});

Then, I changed the 'setup' function in the class to include:

            if (this.determineWhetherClientSupportsWebp(event)) {
                this.outputFormat = 'webp';
            }

@timkelty
Copy link

@soupy1976 can you make a PR with these changes?

@soupy1976
Copy link

@timkelty it seems like I don't have permission. I get the message "It seems you do not have permission to push your changes to this repository" when I try and create the PR.

I haven't really used github before, so I don't know what is wrong.

@timkelty
Copy link

@soupy1976 you need to fork the repo, make the changes on your fork, then create a pull-request: https://help.github.com/en/articles/about-pull-requests

@leviwilson
Copy link

@timkelty is there a trick to getting people to respond to said pull requests? I've created 2x PRs and have yet to receive feedback on either of them.

@timkelty
Copy link

@leviwilson hah. Yeah I can't quite figure out how this project is managed either…

Given that the user base seems more active than the AWS labs maintainers, I wonder if it makes more sense to make a community-driven fork where we can actually fix things and clean things up (dockerizing build!).

I'd be interested if you're up for it. Shoot me an email.

@leviwilson
Copy link

@timkelty you mean with an automated CI pipeline that has a reproducible build? That's crazy talk.

@soupy1976
Copy link

But then we would all miss out on important information from the AWS devs, like for instance if the lambda environment was being updated and going to break this functionality, they would not be able to warn us about it and it would break everyone's production environments leaving people completely screwed. Oh ..... errr, hang on!
:-)

@leviwilson
Copy link

@timkelty adding a .travis-ci.yml wouldn't take much to get this to build I don't think; I could probably PR when I get some time as I've already automated the pipeline for our fork in https://github.com/northwoodspd/serverless-image-handler/blob/sharp-output-format/.gitlab-ci.yml

@timkelty
Copy link

@leviwilson there is also fork/re-construction going on here: https://github.com/venveo/serverless-image-handler

@cgpsn
Copy link

cgpsn commented Sep 11, 2019

Any possibility of this getting merged?

browniebroke pushed a commit to festicket/serverless-image-handler-1 that referenced this issue Sep 24, 2019
@browniebroke
Copy link

We also have this problem, so I took the changes suggested in here and submitted a pull request to try to get them merged. #152

In the meantime, we'll probably build our our own zip of the function or move to another approach like suggested here.

@karensg
Copy link

karensg commented Jan 16, 2020

This would be great. Really excited about this feature.

@beomseoklee
Copy link
Member

We have updated our solution, and I believe your issue has been fixed. If you still see the issue with the latest version (v4.2), please feel free to reopen the issue.

You can refer to the recent changes here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
10 participants