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

Re-enable nextjs compression or allow enabling it optionally #475

Open
janus-reith opened this issue Jul 19, 2024 · 12 comments
Open

Re-enable nextjs compression or allow enabling it optionally #475

janus-reith opened this issue Jul 19, 2024 · 12 comments

Comments

@janus-reith
Copy link

Although Cloudfront does brotli compression on its own, there are good reasons to have the nextjs lambda compress on its own.to avoid hitting the 6 MB Limit.
In my case when using Payload CMS, a 10 MB file shrinks to ~400 kb using the nextjs gzip compression.

This comment mentions that compression which appareantly caused issues in earlier version actually seems fine now:
#35 (comment)

@conico974
Copy link
Collaborator

We probably don't want to re enable this.
First of all, it will only work for routes, not for middleware response or external rewrite.
And it only does gzip compression.

The way to do this properly is probably with a custom wrapper https://open-next.js.org/config/custom_overrides#custom-wrapper

Something like that should do the trick

// customWrapper.ts
import streamingWrapper from 'open-next/wrappers/aws-lambda.js'
import {WrapperHandler} from 'open-next/types/open-next'
 
const handler : WrapperHandler = async (handler, converter) => {
  const defaultHandler = await streamingWrapper.wrapper(handler, converter)
  return async (event: APIGatewayProxyEventV2): Promise<APIGatewayProxyResultV2> => {
    const acceptHeaders = event.headers["accept-encoding"];
    const result = await defaultHandler(event)
    const compressedBody = // Here you compress with node zlib result.body
    return {
      ...result,
     headers: {
       ...result.headers,
      'content-encoding': 'br' // Or whatever
     },
    body: compressedBody
    }
  }
}
 
export default {
  wrapper: handler,
  name: "custom-aws-lambda",
  supportStreaming: false,
};

@janus-reith
Copy link
Author

Thanks a lot for the reply, that seems like the way to go.

I'm using open-next with SST Ion currently, which uses open-next internally.
Do you know by chance how I could add such a wrapper there? I'm still puzzling together some of the pieces and am unsure what's overridable and what not.

@conico974
Copy link
Collaborator

You need a custom open-next.config.ts https://open-next.js.org/config.
How to use it with a custom wrapper is the link i put in the previous comment.
You can close the issue if you've managed to fix this

@janus-reith
Copy link
Author

Thanks, just figured it out.
I'm wondering though, if using the default gzip compression isn't a valid usecase aswell.

First of all, it will only work for routes, not for middleware response or external rewrite.

In my case, thats the only place where I'd need it and where it would have a notable effect.
Would there be an issue with at at least keeping it disabled, but optionally allowing to enable it, either if specified explicitely in your next.config or with some separate flag?

Or do you mean middleware responses or external rewrites would break in case that would be enabled?

@conico974
Copy link
Collaborator

It probably wouldn't break anything in most cases (it might still break things for earlier version of next).
The thing that is not really good is that the same lambda will produce either a compressed response (for route) or an uncompressed one.
I think the proper way to deal with this in OpenNext would be to either create an aws-lambda-compressed wrapper here or to add a flag to enable compression in the default lambda wrapper itself.
This would allow us to properly support deflate, gzip and brotli compression instead of just gzip & deflate

@janus-reith
Copy link
Author

I was able to get gzip compression working and it did solve my lambda response issues, thanks for the hints!

What’s left now is that there seems to be some issue with cookies being swallowed, at least that’s what I think is hapoening since signins to my app fail although there’s a successful response and no cookies are set.

Anz chance there are some known issues with this when a custom wrapper is used? From what I see, my wrapper leaves all existing headers as they are and only adds Content-Encoding and Content-Length

@conico974
Copy link
Collaborator

This should have no effect at all, custom wrapper uses the exact same logic as integrated wrapper.
The only thing that could cause some issue in a custom wrapper would be if you don't properly propagate the response. Lambda expect cookies to be set in a separate cookies field
What version of Next and OpenNext are you using ?
Did you look for logs in cloudwatch ?

@janus-reith
Copy link
Author

janus-reith commented Jul 19, 2024

What version of Next and OpenNext are you using ?
Next 15.0.0-canary.58 currently and open-next 3.0.2

I just tried on a different app which uses Payload CMS aswell where Login / Cookies worked on the deployment to add the same wrapper for compression.
That one has less complexity and therefore didn't scratch the 6 MB response limit yet.
The same happens there, so.I could verify that it's indeed the custom wrapper causing it.

Sorry, this is going a bit beyond the scope of the original issue, but I believe it could also help other people running into response size limits.

This is what I'm using currently. I think I'm passing everything from the orignal response properly.

[...]
import defaultWrapper from 'open-next/wrappers/aws-lambda.js'
import zlib from 'node:zlib'

const handler: WrapperHandler = async (handler, converter) => {
  const defaultHandler = await defaultWrapper.wrapper(handler, converter)

  return async (event: AwsLambdaEvent): Promise<AwsLambdaReturn> => {
    const result = await defaultHandler(event)

    const bodyString = typeof result.body === 'string' ? result.body : JSON.stringify(result.body)

    const compressedBody = zlib.gzipSync(bodyString)

    const response = {
      ...result,
      body: compressedBody.toString('base64'),
      headers: {
        ...result.headers,
        'Content-Encoding': 'gzip',
        'Content-Length': compressedBody.length,
      },
      isBase64Encoded: true,
    }

    return converter.convertTo(response, event)
  }
}

export default {
  name: 'custom-aws-lambda',
  supportStreaming: false,
  wrapper: handler,
}

@janus-reith
Copy link
Author

janus-reith commented Jul 19, 2024

Another detail I just found when comparing the API route for login from a previous regular deployment that did not have the custom wrapper with the one that has it and is now missing the Set-Cookie header:

While the wrapped one now has a gzip encoding header, the regular doesn't have any.
I expected the default behaviour of the Cloudfront distribution to compress with brotli.

This might be related, maybe it's also out of the scope of open-next: sst/ion#723

@conico974
Copy link
Collaborator

I don't see how just using a custom wrapper by itself could cause an issue. The code for importing the wrapper is this https://github.com/sst/open-next/blob/a43fa46e9aeaf18b3c68647d1ef2beca3b05f290/packages/open-next/src/core/resolve.ts#L30-L42
I'm pretty sure you don't need to run the converter again here, that's probably your issue, the defaultHandler is already returning converter.convertTo, and the default one is removing the set-cookie header. Just returing response should be enough

@janus-reith
Copy link
Author

that's probably your issue, the defaultHandler is already returning converter.convertTo, and the default one is removing the set-cookie header.

It indeed was! Again, thanks a lot!

@janus-reith
Copy link
Author

I'll keep this open since I believe adding compression could be more straightforward. Maybe some implementation that would align with the next.config compress option could be helpful to align closer with how nextjs behaves.
It could check if compress is explicitly set to true in your next config and add compression to the lambda based on that (Although that also wouldn't reflect the original next behaviour 1:1 where it's applied by default.)

But of course feel free to close if you think otherwise.

Since the same app that failed due to the respose size worked on Vercel, I assume they also compress at the lambda level, or have some other limits with AWS than regular accounts.

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

No branches or pull requests

2 participants