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

Add support for custom non-html route encoding #4549

Merged
merged 3 commits into from
Sep 9, 2022
Merged

Add support for custom non-html route encoding #4549

merged 3 commits into from
Sep 9, 2022

Conversation

altano
Copy link
Contributor

@altano altano commented Aug 30, 2022

Changes

See #4545 for further discussion of the issue, but the summary is that currently non-html routes have their buffers encoded as utf-8 when being written to disk during an SSG build, which works for some streams but not others. The main use case that isn't working is a PNG file route. The binary stream gets mangled by encoding as utf-8, destroying the PNG image. By allowing a binary encoding, the PNG image is written to disk correctly instead.

NOTE: By causing the API surface of endpoints (which return Response) and ssg routes (which return this custom object) we're making the life of whoever tackles withastro/docs#760 a little harder.

Testing

I have added a placeholder.png.ts route to the examples/non-html-routes example project. This route was previously broken (since there was no way to specify a binary encoding) but now works perfectly. Just run pnpm build in this example dir and observe that the dist/placeholder.png file is valid and can be previewed.

Docs

I've submitted a PR to update the docs: withastro/docs#1434

Alternative

Let the body be string | Buffer instead of string. When writing a Buffer, fs.writeFile ignores the encoding option.

Why I chose current implementation instead: the ergonomics of allowing both string and Buffer might be easier on the user, but we shouldn't force people to create a Buffer (if they don't already have one) to override the encoding.

@changeset-bot
Copy link

changeset-bot bot commented Aug 30, 2022

🦋 Changeset detected

Latest commit: 0e49bf2

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

This PR includes changesets to release 14 packages
Name Type
astro Minor
@e2e/astro-component Patch
@e2e/error-react-spectrum Patch
@e2e/error-sass Patch
@e2e/errors Patch
@e2e/hydration-race Patch
@e2e/lit-component Patch
@e2e/preact-component Patch
@e2e/react-component Patch
@e2e/solid-component Patch
@e2e/solid-recurse Patch
@e2e/svelte-component Patch
@e2e/e2e-tailwindcss Patch
@e2e/ts-resolution 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 pkg: astro Related to the core `astro` package (scope) pkg: example Related to an example package (scope) labels Aug 30, 2022
@matthewp
Copy link
Contributor

Thanks! Change looks good overall. Instead of having an example which I don't think people would use, could you move that code into a test at packages/astro/test/fixtures/?

@github-actions github-actions bot removed the pkg: example Related to an example package (scope) label Aug 31, 2022
@altano
Copy link
Contributor Author

altano commented Aug 31, 2022

Code updated:

  • example removed
  • test added (both .json and .png pages)
  • object spread replaced with explicit assignment

@matthewp
Copy link
Contributor

@altano thank you! Everything looks good, this will be merged when we are ready to do our next minor release.

@matthewp matthewp added the semver: minor Change triggers a `minor` release label Aug 31, 2022
@matthewp matthewp merged commit 255636c into withastro:main Sep 9, 2022
@astrobot-houston astrobot-houston mentioned this pull request Sep 9, 2022
@altano altano deleted the alan/non-html-route-encoding branch September 10, 2022 05:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants