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 filterSerializedResponseHeaders function #6569

Merged
merged 8 commits into from Sep 5, 2022
Merged

Add filterSerializedResponseHeaders function #6569

merged 8 commits into from Sep 5, 2022

Conversation

Rich-Harris
Copy link
Member

Companion to #6565. Supersedes #6541, closes #1971.

This is a breaking change — it prevents response headers that come from a fetch in load from being serialized into the HTML, unless they are explicitly included.

Please don't delete this checklist! 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 pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Sep 4, 2022

🦋 Changeset detected

Latest commit: 643f664

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

@netlify
Copy link

netlify bot commented Sep 4, 2022

Deploy Preview for kit-demo ready!

Name Link
🔨 Latest commit c86e238
🔍 Latest deploy log https://app.netlify.com/sites/kit-demo/deploys/6316074a2937a20008203613
😎 Deploy Preview https://deploy-preview-6569--kit-demo.netlify.app/build
📱 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.

@@ -143,7 +143,7 @@ export async function load({ depends }) {
- it can be used to make credentialed requests on the server, as it inherits the `cookie` and `authorization` headers for the page request
- it can make relative requests on the server (ordinarily, `fetch` requires a URL with an origin when used in a server context)
- internal requests (e.g. for `+server.js` routes) go direct to the handler function when running on the server, without the overhead of an HTTP call
- during server-side rendering, the response will be captured and inlined into the rendered HTML
- during server-side rendering, the response will be captured and inlined into the rendered HTML. Note that headers will _not_ be serialized, unless explicitly included via [`filterSerializedResponseHeaders`](/docs/hooks#handle)
Copy link
Member

Choose a reason for hiding this comment

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

Are we not automatically whitelisting any headers anymore? I remember in some version of this proposal, we would automatically include content-type. That's gone?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was whitelisted because some tests expected it, but that seems like a bad reason to keep it. Things like response.json() work the same way with or without content-type, so the only time you'd actually need it is if you're explicitly reading that header.

I did wonder about filtering the headers during SSR so that you'd get a 'btw you need to include this header' error

Copy link
Member Author

Choose a reason for hiding this comment

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

(though now that I think about it i have no idea if it's possible to monkey-patch response.headers like that)

Copy link
Member Author

Choose a reason for hiding this comment

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

the monkey has been patched

Rich-Harris and others added 2 commits September 5, 2022 11:03
Co-authored-by: Conduitry <git@chor.date>
Co-authored-by: Conduitry <git@chor.date>
@bummzack
Copy link
Contributor

Not forwarding the content-type header might create a lot of unwanted issues/side-effects with other libraries. It took me quite some time to figure out why the graphql-request library suddenly started erroring, even though the response was there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blacklist fetch response headers when serialised for initial fetch
4 participants