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

Headers are not set when ssr = false #8428

Closed
ebeloded opened this issue Jan 10, 2023 · 6 comments · Fixed by #9352
Closed

Headers are not set when ssr = false #8428

ebeloded opened this issue Jan 10, 2023 · 6 comments · Fixed by #9352
Labels
bug Something isn't working ready to implement please submit PRs for these issues!
Milestone

Comments

@ebeloded
Copy link

ebeloded commented Jan 10, 2023

Describe the bug

In production build, the headers are not set if ssr = false in +layout.server.ts

Reproduction

  1. Create SvelteKit skeleton project

  2. Set headers in hooks.server.ts

export const handle: Handle = async ({ event, resolve }) => {
  event.setHeaders({
    'x-foo': 'bar',
    'cache-control': 'no-store',
  })
  return resolve(event)
}
  1. Disable SSR in +layout.server.ts:
export const ssr = false
  1. pnpm build
  2. pnpm preview
  3. Get headers (https -h http://127.0.0.1:4173/)
  4. Note the missing headers

See reproduction repo: https://github.com/ebeloded/kit-headers-bug

Logs

❯ http -h http://127.0.0.1:4173/                      
HTTP/1.1 200 OK
Access-Control-Allow-Origin: *
Connection: keep-alive
Date: Tue, 10 Jan 2023 01:58:08 GMT
Keep-Alive: timeout=5
Transfer-Encoding: chunked
content-type: text/html
etag: "1673315883560"

System Info

System:
    OS: macOS 13.1
    CPU: (10) arm64 Apple M1 Pro
    Memory: 370.16 MB / 32.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 16.17.1 - ~/.nvm/versions/node/v16.17.1/bin/node
    npm: 8.15.0 - ~/.nvm/versions/node/v16.17.1/bin/npm
  Browsers:
    Chrome: 108.0.5359.124
    Firefox: 108.0.1
    Safari: 16.2
  npmPackages:
    @sveltejs/adapter-auto: ^1.0.0 => 1.0.0 
    @sveltejs/adapter-node: ^1.1.0 => 1.1.0 
    @sveltejs/kit: ^1.0.0 => 1.0.10 
    svelte: ^3.54.0 => 3.55.0 
    vite: ^4.0.0 => 4.0.4

Additional Information

  1. The headers are returned when in dev mode pnpm dev
  2. In the reproduction, I use adapter-auto, but behavior is the same with adapter-node
@dummdidumm dummdidumm self-assigned this Jan 10, 2023
@dummdidumm
Copy link
Member

What do you mean by the step "get headers (https -h http://127.0.0.1:4173/)"? When I'm invoking the URL in the browser and look at the headers in the network tab, they are shown as expected.

@dummdidumm dummdidumm removed their assignment Jan 10, 2023
@ebeloded
Copy link
Author

@dummdidumm sorry about the confusion. The https comes from HTTPie.

I've double-checked and confirmed the issue.

Here is the screenshot of headers when the app running in dev mode (pnpm run dev). The headers are present:

And here is a screenshot of the app that runs in production mode (pnpm run preview). The headers are absent:

Please let me know if you get different results.

The problem I am trying to solve in my app is to set the cache-control header, but it proves to be impossible when ssr = false.

@Rich-Harris
Copy link
Member

ohhh.... this must be because of #8131.

Two options:

  1. we treat this as a bug and revert [breaking] prerender shells when ssr false and prerender not false #8131
  2. add export const prerender = false alongside export const ssr = false

#8131 is a valuable optimisation so it would be a shame to lose it, but I wonder how we can make this behaviour clearer.

@dummdidumm
Copy link
Member

Is there the possibility of a third option where we somehow check if setHeaders is invoked and opt out of prerendering in that case, too, or would that contain too many false positives?

@Rich-Harris
Copy link
Member

No, that seems like a pretty good solution as long as it's documented (you could always opt back in with prerender = true). There's still the possibility that someone could do response.headers.set(...) after calling resolve in handle — don't know if we'd want to deoptimize in that case as well

@Rich-Harris Rich-Harris added bug Something isn't working ready to implement please submit PRs for these issues! labels Jan 28, 2023
@Rich-Harris Rich-Harris added this to the soon milestone Jan 28, 2023
dummdidumm pushed a commit that referenced this issue Mar 8, 2023
reverts #8131
This reverts the optimisation wherein pages with ssr = false and no explicit prerender = false would automatically be prerendered, as it turns out to be unsafe.

fixes #9275
fixes #8428
@benmccann
Copy link
Member

There's a feature request for saving the headers and serving them here: #9408

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ready to implement please submit PRs for these issues!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants