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

Include charset=utf-8 by default in application/json endpoints. #1669

Merged
merged 6 commits into from
Jul 3, 2021

Conversation

coreh
Copy link
Contributor

@coreh coreh commented Jun 11, 2021

Hey there 👋

I was getting Mojibake when returning unicode data from my endpoints, so I checked the headers and noticed the charset=utf-8 option was missing from Content-Type. This in turn was causing the response to incorrectly be interpreted as Windows-1252.

This PR changes src/runtime/server/endpoint.js to include that by default for typeof body === 'object'.

I couldn't find any tickets about this, believe this change is simple enough that it's okay for me to open a PR without filing a ticket first.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts

@benmccann
Copy link
Member

It looks like there are some CI failures

@Conduitry
Copy link
Member

Yep, this seems reasonable to do for responses that SvelteKit is automatically serializing.

The test failures look like just some content-type values in other tests that need to be updated to include the charset.

@coreh
Copy link
Contributor Author

coreh commented Jun 11, 2021

Sorry, it was quite late and my brain was foggy, I think I ran tests locally on the wrong directory. 🥶

I'll fix these after work, later today

@changeset-bot
Copy link

changeset-bot bot commented Jun 11, 2021

🦋 Changeset detected

Latest commit: c8cced1

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

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

@coreh
Copy link
Contributor Author

coreh commented Jun 12, 2021

@benmccann @Conduitry Fixed linting and the broken test.

I also removed the value check for the response body in c8cced1, since upon further digging it looks like the tests don't actually encode/decode the strings as byte buffers like a real HTTP implementation would, so it wasn't really testing anything.

@coreh
Copy link
Contributor Author

coreh commented Jun 22, 2021

Hey 👋

Let me know if you need me to make any additional changes or add more tests.

I can add back the test for UTF-8 encoding/decoding if there's a sanctioned way to make the HTTP response binary during the tests (or I can also try to figure out a mechanism for this if not present)

@benmccann benmccann merged commit 939188e into sveltejs:master Jul 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants