cdk-nextjs icon indicating copy to clipboard operation
cdk-nextjs copied to clipboard

Caching of static pages

Open franciscorubin opened this issue 2 years ago • 14 comments

After some testing I noticed that pages are not being cached, so each time they're accessed they trigger the ServerHandler lambda, which makes requests slower. Couldn't we cache static pages just like any other resources and serve them from CloudFront cache?

franciscorubin avatar Nov 14 '22 08:11 franciscorubin

https://github.com/jetbridge/cdk-nextjs/blob/861c86da30943043e24390ddc81a6583d59c0307/src/Nextjs.ts#L160

https://github.com/jetbridge/cdk-nextjs/blob/861c86da30943043e24390ddc81a6583d59c0307/src/Nextjs.ts#L443

Is there any particular reason you disabled the cache on the lambda functions?

franciscorubin avatar Nov 14 '22 15:11 franciscorubin

The line you linked has the cachePolicy for the lambda (eg api), which we don't want to cache.

https://github.com/jetbridge/cdk-nextjs/blob/main/src/Nextjs.ts#L493, maybe something is wrong here? Would changing it to... help?

'/': hasIndexHtml ? staticBehavior : lambdaBehavior ,

khuezy avatar Nov 14 '22 16:11 khuezy

Ah I see, if the same lambda is used for the API it cannot be cached.

I've done some more testing, and got the following results:

Using cdk-nextjs:

  • the html page download spends around 500ms waiting for a server response, and is not cached on CF. This time is much higher when the lambda function is cold (first attempt took 1.5s just waiting for a response)
  • the rest of the resources are cached, and take 30-60ms waiting for a server response.

Using vercel:

  • all the resources (including the html page) take 30-60ms waiting for a server response, so I assume they are all cached at the CDN level

A possible solution could be to store the static html pages in S3 as it's done with the resources. Then they will use the S3 Origin and benefit from the caching policies. But I'm not sure how that would work, as the path for the pages is different from the resource paths ('/' instead of '/_next/server/pages/index.html').

franciscorubin avatar Nov 14 '22 18:11 franciscorubin

The line you linked has the cachePolicy for the lambda (eg api), which we don't want to cache.

https://github.com/jetbridge/cdk-nextjs/blob/main/src/Nextjs.ts#L493, maybe something is wrong here? Would changing it to... help?

'/': hasIndexHtml ? staticBehavior : lambdaBehavior ,

I think this would help only on the /index.html page, but not on the others generated by nextjs.

franciscorubin avatar Nov 14 '22 18:11 franciscorubin

Thanks for doing the bench testing. I'm sure there's plenty to optimize. I'm been mostly testing local stuff.

khuezy avatar Nov 14 '22 18:11 khuezy

Happy to help!

I looked at how serverless-next.js does this, and it seems to be the solution I mentioned above:

  • moving pages into static-pages folder: https://github.com/serverless-nextjs/serverless-next.js/blob/ca7f7f72e3b1e5fcb49ffc4a3f6d6f629e644b20/packages/libs/core/src/build/builder.ts#L411
  • uploading static-pages folder content into S3 for caching: https://github.com/serverless-nextjs/serverless-next.js/blob/83805bb56d824ff3777e3da6591078ffd7053f48/packages/libs/s3-static-assets/src/index.ts#L88

Basically they just copy all the html files from the nextjs output folder into a separate folder and then upload them to S3 to the root path.

franciscorubin avatar Nov 14 '22 18:11 franciscorubin

@revmischa, what do you think?

khuezy avatar Nov 14 '22 19:11 khuezy

I believe if you want to cache lambda responses it should be possible. You can specify what caching policy you want as part of the distribution config and enable it. I think we can enable GET/HEAD caching but with a default 0 TTL.

The idea is that you should be able to cache API responses, but you need to explicitly set the caching headers in your API response if you want that. I think s-maxage might be the cache-control option that tells CloudFront to cache it for everyone?

For static pages which are handled by NextJS I don't know what the best option is. I am not sure if sls-next.js's approach works with the standalone version of the output, it doesn't seem to output static HTML files. At least not in my setup.

revmischa avatar Nov 14 '22 20:11 revmischa

@pancho111203 The PR above adds caching to static pages. I'll be thoroughly testing this out before I mark it ready.

khuezy avatar Dec 04 '22 22:12 khuezy

@khuezy That's great to hear! I will re-do the bench testing and post it here as soon as it's merged.

franciscorubin avatar Dec 08 '22 21:12 franciscorubin

@pancho111203 are you using the new appDir or the old pages directory? If using the pages, are you exporting getStaticProps? You should be getting cached pages if you are exporting getStaticProps

khuezy avatar Dec 09 '22 17:12 khuezy

@khuezy In the tests I did a while ago I was using the pages dir without any static props, just a very basic page taken from a boilerplate, without any props.

franciscorubin avatar Dec 09 '22 18:12 franciscorubin

So you're saying that adding static props to the page enables caching?

franciscorubin avatar Dec 09 '22 18:12 franciscorubin

Yes, give that a shot. It should add cache-control w/ some max-age. You can return { props: {...}, revalidate: 60} to add ISR. I'm not 100% sure this standalone works like Vercel though... Vercel's non static pages loads in under 50ms, while ours takes 500-700 (maybe b/c they use edge lambdas and we don't)

khuezy avatar Dec 09 '22 18:12 khuezy

I think this issue has been fixed. Please re-open if you think otherwise. Thank you!

bestickley avatar Oct 23 '23 13:10 bestickley