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

Adds support base64 encoding in Netlify Functions #3592

Merged
merged 5 commits into from
Jun 15, 2022
Merged

Conversation

tony-sull
Copy link
Contributor

Changes

Closes #3586

This updates the @astrojs/netlify/functions adapter to check if response body is a base64 encoded string. This is needed to make sure the isBase64Encoded flag is returned if necessary

Because Astro's API endpoints are expected to return a standard Response object, the adapter doesn't have access to the raw BodyInit object that was used. The solution here is to encode the response body in the API endpoint, allowing the adapter to check whether response.text() is a base64 string.

Testing

Added a test suite to verify that base64 responses are returned as expected

Docs

N/A

@changeset-bot
Copy link

changeset-bot bot commented Jun 14, 2022

🦋 Changeset detected

Latest commit: 9b0d889

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

This PR includes changesets to release 1 package
Name Type
@astrojs/netlify 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

@github-actions github-actions bot added the pkg: integration Related to any renderer integration (scope) label Jun 14, 2022
@tony-sull tony-sull marked this pull request as ready for review June 14, 2022 15:54
@matthewp
Copy link
Contributor

Is the use-case about images here? I'm worried about this regular expression and applying it to every request. I'm thinking that it shouldn't be on the developer to base64 encode their images, but rather just include the right Content-Type. And then the adapter could be the one to apply the base64 encoding.

It's a matter of the base64 encoding being an implementation detail of the adapter and not something the application should need to concern itself with.

Also consider cases where you base64 something and want it to be delivered as a string (using text/plain). Just feels like using the mimetype here is a better approach. Happy to discuss.

Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Nice fix.

@natemoo-re natemoo-re self-requested a review June 14, 2022 19:11
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially approved this but on second-thought I agree with @matthewp's suggestion that we should handle this invisibly if at all possible.

@tony-sull
Copy link
Contributor Author

Synced with Matthew offline and think we landed on a better approach that avoids the rather nasty regex 👍

Updated PR to follow!

Copy link
Contributor

@bholmesdev bholmesdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs changeset but LGTM!

@tony-sull
Copy link
Contributor Author

@matthewp This one's ready for another review with the non-regex solution we talked about yesterday 👍

@tony-sull tony-sull requested a review from natemoo-re June 15, 2022 17:17
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Smart change, this is way better. LGTM!

Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@bholmesdev bholmesdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

baseically perfect 👏

@tony-sull tony-sull merged commit 0ddcef2 into main Jun 15, 2022
@tony-sull tony-sull deleted the fix/netlify-base64 branch June 15, 2022 19:49
@github-actions github-actions bot mentioned this pull request Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 BUG: @astrojs/netlify/functions doesn't support base64 encoded responses
4 participants