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

docs(endpoints): added new example for returning buffers from endpoints #3154

Merged
merged 6 commits into from
Jun 4, 2023
Merged

docs(endpoints): added new example for returning buffers from endpoints #3154

merged 6 commits into from
Jun 4, 2023

Conversation

haasal
Copy link
Contributor

@haasal haasal commented May 3, 2023

What kind of changes does this PR include?

New content/code-snippet in the docs

Description

The PR adresses this closed issue.
When using the encoding: 'binary' providers like vercel send the buffer as plain text instead of a binary png. This can be prevented by using the Response object (just like in other frameworks like svelte)

When using the encoding: 'binary' providers like vercel send the buffer as  plain text instead of a binary png. This can be prevented by using the Response object (just like in other frameworks like svelte)
@netlify
Copy link

netlify bot commented May 3, 2023

Deploy Preview for astro-docs-2 ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 7ba29d5
🔍 Latest deploy log https://app.netlify.com/sites/astro-docs-2/deploys/647c9d723f529d00087072e0
😎 Deploy Preview https://deploy-preview-3154--astro-docs-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Thanks for contributing this! I followed your and @Princesseuh's conversation trail, and I tried to make the note more direct.

Erika, can you please fact-check this? And, I know you mentioned something about adding documentation about this. Would you consider this note sufficient, or do you think we should consider replacing the existing example with one that returns a Response? Is that something that will work in all situations, so is maybe more universal to show?

@@ -40,6 +40,18 @@ export async function get({ params, request }) {
}
```

This method of returning a buffer as a response might not work for certain providers like vercel because the `Content-Type` header cannot be specified. For returning images prefer the following way (i.e. return a `Response`-Object):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This method of returning a buffer as a response might not work for certain providers like vercel because the `Content-Type` header cannot be specified. For returning images prefer the following way (i.e. return a `Response`-Object):
Certain providers require the `Content-Type` header to return an image. In this case, return a `Response` object instead which allows you to specify a `headers` property.
For example, to produce a `.png` image on Vercel:

@sarah11918 sarah11918 requested a review from Princesseuh May 3, 2023 19:15
@haasal
Copy link
Contributor Author

haasal commented May 3, 2023

In my opinion the example I added should be the default way to return Buffers in general. Maybe we should replace the 'binary' example completely (as it doesn't work). Making this work has cost me days and this could be a huge deterrent for anyone that tries to use s3 buckets or any other form of buffered images.

@sarah11918 sarah11918 added the improve or update documentation Enhance / update existing documentation (e.g. add example, improve description, update for changes) label May 3, 2023
@sarah11918
Copy link
Member

I'm going to ask @delucis what he thinks about this!

@sarah11918 sarah11918 requested a review from delucis May 12, 2023 11:39
@delucis
Copy link
Member

delucis commented May 18, 2023

Thanks for your patience on this this @haasal! Looking at the original astro issue, it looks like you were experiencing this with an SSR adapter, right?

I believe the current example is for static scenarios (where Astro needs to write the endpoint to the filesystem while building). I’m not sure if we actually support returning Response in static mode or not. We should double check Response works in a static scenario and if it doesn’t document clearly these two situations: 1) object with body and encoding in static mode and 2) Response in SSR.

@haasal
Copy link
Contributor Author

haasal commented May 19, 2023

Yes. Now I see it too. However it mentions nowhere in the docs how to do it with SSR on, which is really important (I.e. s3 buckets). So I would simply add my paragraph/example slightly modified to the SSR part of the endpoint guide

@sarah11918
Copy link
Member

@haasal Would you be willing to update this PR to modify the paragraph and example slightly, as you suggested, and move it to the SSR section?

@haasal
Copy link
Contributor Author

haasal commented May 20, 2023

Sure. I'll add the example to the SSR section.

@sarah11918
Copy link
Member

Hi @haasal - Just checking in here to see whether you're still up for this! I'm sorry, I've been so swamped getting next weeks' big launch docs together for our new features, I've been a little slow to follow up on community PRs.

But we would be so happy to have your contribution, if you're still interested!

@haasal
Copy link
Contributor Author

haasal commented Jun 3, 2023

I think this should be ready. I messed up something with git along the way. Please check if everything is correct before merging.

@sarah11918 sarah11918 merged commit 046e0af into withastro:main Jun 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improve or update documentation Enhance / update existing documentation (e.g. add example, improve description, update for changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants