kit icon indicating copy to clipboard operation
kit copied to clipboard

feat: add content-length headers to generated responses, via new `text(...)` helper

Open Rich-Harris opened this issue 3 years ago • 6 comments

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 test and lint the project with pnpm lint and pnpm 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 changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

Rich-Harris avatar Jan 06 '23 17:01 Rich-Harris

🦋 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

changeset-bot[bot] avatar Jan 06 '23 17:01 changeset-bot[bot]

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)

vercel[bot] avatar Jan 07 '23 18:01 vercel[bot]

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

Rich-Harris avatar Jan 09 '23 16:01 Rich-Harris

@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.

bjon avatar Jan 09 '23 22:01 bjon

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.

Conduitry avatar Jan 09 '23 23:01 Conduitry

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?

Rich-Harris avatar Jan 10 '23 00:01 Rich-Harris