feat: add content-length headers to generated responses, via new `text(...)` helper
closes #5749.
This is just an idea; I don't know if it's the right one. In order to make it easier to determine response content length inside handle, for logging purposes, this adds a new text(body) helper similar to the json(data) helper. It returns a Response object with a content-length header, and uses it in favour of new Response(...) when generating pages and other responses.
One observation: it'd be nice if we could unit-test Server rather than having Playwright tests for the bulk of the tests in server.test.js. Might look into that separately.
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
- [x] 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
- [x] This message body should clearly illustrate what problems it solves.
- [x] Ideally, include a test that fails without this PR but passes with it.
Tests
- [x] Run the tests with
pnpm testand lint the project withpnpm lintandpnpm check
Changesets
- [x] If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running
pnpm changesetand following the prompts. All changesets should bepatchuntil SvelteKit 1.0
🦋 Changeset detected
Latest commit: 4e360221d1bc51c842474137688854549a0abd3f
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 | Minor |
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
The latest updates on your projects. Learn more about Vercel for Git ↗︎
| Name | Status | Preview | Updated |
|---|---|---|---|
| kit | 🔄 Building (Inspect) | Jan 9, 2023 at 9:32AM (UTC) | |
| kit-svelte-dev | 🔄 Building (Inspect) | Jan 9, 2023 at 9:32AM (UTC) |
Ah, that's a good point. I would have assumed that if something in between SvelteKit and the user were to compress the body it would generate a new response with new headers, but I'll freely confess my ignorance here. Safest thing would be to close #5749 as wontfix but that seems unsatisfying.
Am inclined to just leave this here until someone with more authority weighs in
@dummdidumm Google's dynamic compression (CDN) requires the response to have content-length. Without it, no compression is done. https://cloud.google.com/cdn/docs/dynamic-compression#response-not-compressed
It's was no problem to add content-length in sveltekit manually. Maybe the text() and json() could have an optional addContentLength: boolean arg, or something similar? It would be great to have kit handle it.
If there's some library or something that compresses responses on the fly but doesn't correctly update the content-length header, that's certainly a bug in that library. I think we should only worry about that if there were some extremely popular library that did that that seemed extremely unlikely to want to fix that bug.
I don't think there's much value in having an addContentLength option — it doesn't cost anything extra for us, and I can't imagine any cases where you wouldn't want the header. Honestly I'm a little surprised it doesn't get added automatically (if you do new Response('hello') it'll add a content-type: text/plain;charset=UTF-8 header, but not content-length — what the hell?)
So it sounds like the consensus is in favour of merging this?