sst / open-next

Open source Next.js serverless adapter
https://open-next.js.org
MIT License
3.73k stars 112 forks source link

Changes encoding on cache.body from utf8 to base64 #329

Closed dylanirion closed 6 months ago

dylanirion commented 7 months ago

Code generated icons are cached as routes with a binary body. open-next currently merges the route meta and utf-8 encoded body into a single *.cache file, which can be problematic for binary png data. This PR simply changes the encoding on the cached route body from utf8 to base64 to preserve binary data.

Alternatively, we could preserve the utf-8 encoding on json routes, and only apply base64 when necessary according to meta.headers.content-type?

changeset-bot[bot] commented 7 months ago

🦋 Changeset detected

Latest commit: b4c7efeae289264ba7b46c5a5ee7320834ef4c2d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages | Name | Type | | ---------------- | ----- | | open-next | Patch | | app-pages-router | Patch | | app-router | Patch | | tests-unit | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel[bot] commented 7 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
open-next ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 5, 2024 3:20pm
khuezy commented 7 months ago

Thanks!

dylanirion commented 7 months ago

Did you get a chance to deploy the changes and verify things are working correctly? My only concern is that we now default to base64 and only set utf8 under 1 specific condition (content type is application/json)

icons and a dummy static route that returns json are working in my deployment. I hear you RE the greedy base64 encoding. We could do the opposite and catch via startsWith("image") instead? I nearly did this but my thinking was it's cleaner to match just json over the countless other binary mime types that users might be generating. I considered text/html but figured that's what a page is for. I suppose matching text/plain might be necessary then too.

Would the better pattern beutf8 as default and then specific cases for base64? Happy to re-commit!

khuezy commented 7 months ago

Yes I think we should default as utf8 and only do base64 for images. Sorry for the churn, but it's good that we have this discussion with regards to bandwidth... it does add up at scale.

dylanirion commented 7 months ago

Yes I think we should default as utf8 and on do base64 for images. Sorry for the churn, but it's good that we have this discussion with regards to bandwidth... it does add up at scale.

Good point, commit incoming!

conico974 commented 7 months ago

We should probably just use this function here https://github.com/sst/open-next/blob/ba6e176fe30c3654de24c34a0269cbf1ac4d8ada/packages/open-next/src/adapters/binary.ts#L59C27-L59C27 We already use this function in other places, we should have a single way to choose between utf8 and base64

dylanirion commented 7 months ago

We should probably just use this function here https://github.com/sst/open-next/blob/ba6e176fe30c3654de24c34a0269cbf1ac4d8ada/packages/open-next/src/adapters/binary.ts#L59C27-L59C27 We already use this function in other places, we should have a single way to choose between utf8 and base64

Hah! How'd I miss that? I'll drop this in with another commit