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

HTTP Header directive not used for RSC requests #100

Open
6 tasks done
aheissenberger opened this issue Dec 18, 2024 · 7 comments
Open
6 tasks done

HTTP Header directive not used for RSC requests #100

aheissenberger opened this issue Dec 18, 2024 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@aheissenberger
Copy link
Contributor

aheissenberger commented Dec 18, 2024

Describe the bug

When I add a http header to a page, the header is added to the page request but not to the RSC request.
This is specifically a problem for HTTP Cache headers which define how long a content can be cached on the client side and at the proxy (CDN).

I am not sure if there are other header which need to be set on RSC streams. Maybe a simple optional option {"rsc":true} on the existing function could solve this:

headers(
  { "Cache-Control": "s-maxage=1" } ,
  {"rsc":true}
);

Or maybe a specific cacheHeader function?

It looks like you currently merge the new entry to the existing one as the result is:
Cache-Control: must-revalidate, s-maxage=1

Maybe a second option to define this would help to replace the must-revalidate:

headers({
      "Cache-Control": "s-maxage=1",
    },{"replace":true});

Reproduction

https://d53oahuh7w7td.cloudfront.net/s/page/hello

Steps to reproduce

  1. add a cache header to a non static page:
  import { headers } from "@lazarv/react-server";

  export default async function HelloPage() {
    headers({
      "Cache-Control": "s-maxage=1",
    });
  
    return (
  1. request the page e.g. https://d53oahuh7w7td.cloudfront.net/s/page/hello - this is OK:
    image

  2. go to an other page and come back - or load this url:
    image

System Info

System:
    OS: macOS 15.2
    CPU: (12) arm64 Apple M4 Pro
    Memory: 3.07 GB / 48.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 23.4.0 - /opt/homebrew/bin/node
    Yarn: 1.22.22 - /opt/homebrew/bin/yarn
    npm: 10.9.2 - /opt/homebrew/bin/npm
    pnpm: 9.15.0 - /opt/homebrew/bin/pnpm
    bun: 1.1.40 - /opt/homebrew/bin/bun
  Browsers:
    Chrome: 131.0.6778.140
    Safari: 18.2
    Safari Technology Preview: 18.2
  npmPackages:
    @lazarv/react-server: workspace:^ => 0.0.0 
    @lazarv/react-server-adapter-aws: workspace:^ => 0.0.0

Used Package Manager

pnpm

Logs

No response

Validations

@aheissenberger aheissenberger added the pending triage Pending triage label Dec 18, 2024
@lazarv lazarv added bug Something isn't working and removed pending triage Pending triage labels Dec 19, 2024
@lazarv lazarv self-assigned this Dec 19, 2024
@lazarv
Copy link
Owner

lazarv commented Dec 19, 2024

The issue is that the default Cache-Control header is set by using the lowercase key cache-control, while the headers() function used a non-lowercased version of it, resulting in multiple entries in the headers object with different casing, like this:

{
  "cache-control": "must-revalidate",
  "Cache-Control": "s-maxage=1"
}

I was not able to reproduce any issues with the headers regarding the response type being HTML or RSC. In both cases, the headers were applied properly. But when the page or RSC is static exported, these headers will not be applied. This could be an enhancement in the SSG feature set: store the response headers besides the HTML or RSC payload and use that information when serving the static files. If this is the requested feature, then I would suggest to move the feature request into a separate issue.

@aheissenberger
Copy link
Contributor Author

there is no need to set header for static rendered pages

RSC Cache-Control
The response of this url https://d53oahuh7w7td.cloudfront.net/s/page/hello is dynamical rendered by the react-server framework as you can see at the timestamp in the page content changes on each request.
The repose of the related RSC Uri https://d53oahuh7w7td.cloudfront.net/s/page/hello/rsc.x-component should provide the same timestamp changes on each request.

But because the header in the RSC response is missing the additonal entry s-maxage=1 in the Cache-control and no other cache directive e.g. s-max-age=0, the CDN sends the cached value and does not forward the request to the lambda function.

@lazarv
Copy link
Owner

lazarv commented Dec 19, 2024

Can you reproduce this issue when running the app locally using Node.js?

I'm testing this with a minimal component:

import { headers } from "@lazarv/react-server";

export default function App() {
  headers({
    "cache-control": "s-maxage=1",
  });

  return <div>Hello World</div>;
}

Just running: pnpm react-server ./App.jsx or pnpm react-server start after building with pnpm react-server build ./App.jsx
Calling this endpoint: http://localhost:3000/rsc.x-component

Response headers are:

< cache-control: s-maxage=1
< content-type: text/x-component; charset=utf-8
< last-modified: Thu, 19 Dec 2024 12:58:06 GMT
< Date: Thu, 19 Dec 2024 12:58:06 GMT
< Connection: keep-alive
< Keep-Alive: timeout=5
< Transfer-Encoding: chunked

Works both in dev and a production build.

@aheissenberger
Copy link
Contributor Author

I changed my code to use the lowercase version of cache-control and it replaced the existing entry.

But your own react-server header docu and the official moz docs use the Pascal-Case names.

It should be handled by header() function as according to the HTTP/1.1 specification (RFC 7230, Section 3.2), header field names are treated as case-insensitive.

Based on the same specs multiple headers with the same name - e.g.:

{
  "cache-control": "must-revalidate",
  "Cache-Control": "s-maxage=1"
}

are merged when the header supports multiple entries which is correct for Cache-Control but not for cookies where multiple headers with the same name Set-Cookie are allowed. Using an Object is not the best structure to handle a list of headers. Is there no standard library to handle alle the cases?

Why not use the office Javascript Header Object which handles this with .append() and .set()?

@lazarv
Copy link
Owner

lazarv commented Dec 19, 2024

Handling cookies is a very specific use case, there's an API for it just exposing the HatTip cookie store. https://react-server.dev/framework/http#cookies

In the linked PR, there's a minimal resolution to the case-sensitivity issue, which only affects headers that are also handled by the framework and not just the developer using it, in cases like Cache-Control or Content-Type and only when using SSR (both in HTML or RSC).

But you're right regarding the store for the headers as the case-sensitivity issue could be an error in the app using the framework, not just inside the framework and the framework should prevent this issue. So I'm thinking about a new API similar to the cookie handling functions, setHeader, appendHeader and deleteHeader and it will use the Header object to store and to merge the response headers when sending the response during SSR rendering. This is not an issue on the API route level or in middlewares as it's totally up to the developer to send a new Response in these use cases.

@aheissenberger
Copy link
Contributor Author

I found the problem, I build my apps with defining root in the react-server.config.json:

{
  "root": "src"
}

you find my example here:
https://github.com/aheissenberger/rs-header-cache-Kopie

react-server build
react-server start

get the RSC response with wrong Cache-Control header:

curl 'http://localhost:3000/s/page/hello/rsc.x-component' \
  -H 'Accept: text/x-component,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.7' -vv
* Host localhost:3000 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
*   Trying [::1]:3000...
* Connected to localhost (::1) port 3000
> GET /s/page/hello/rsc.x-component HTTP/1.1
> Host: localhost:3000
> User-Agent: curl/8.7.1
> Accept: text/x-component,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.7
> 
* Request completely sent off
< HTTP/1.1 200 OK
< cache-control: must-revalidate
< content-type: text/x-component; charset=utf-8
< last-modified: Thu, 19 Dec 2024 17:49:42 GMT
< Date: Thu, 19 Dec 2024 17:49:42 GMT
< Connection: keep-alive
< Keep-Alive: timeout=5
< Transfer-Encoding: chunked
< 
0:[[],"$L1"]
1:[[],["$","div",null,{"children":"Hello"}]]
* Connection #0 to host localhost left intact

I can also replicate the problem if I build an app without specifying a root file when calling react-server build. The app will not response to / but to all other roots but without sending the correct cache header.

I can even reproduce the problem without using the react-server.config.json config:

  1. add a second page called hello.jsx to your example
  2. run build without specifying a root file pnpm react-server build
  3. start the app pnpm react-server start
curl 'http://localhost:3000/hello/rsc.x-component' \
-H 'Connection: keep-alive' \
-H 'React-Server-Outlet: PAGE_ROOT' \
-H 'Referer: http://localhost:3000/' \
-H 'accept: text/x-component' \
-vv
* Host localhost:3000 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
*   Trying [::1]:3000...
* Connected to localhost (::1) port 3000
> GET /hello/rsc.x-component HTTP/1.1
> Host: localhost:3000
> User-Agent: curl/8.7.1
> Connection: keep-alive
> React-Server-Outlet: PAGE_ROOT
> Referer: http://localhost:3000/
> accept: text/x-component
> 
* Request completely sent off
< HTTP/1.1 200 OK
< cache-control: must-revalidate
< content-type: text/x-component; charset=utf-8
< last-modified: Thu, 19 Dec 2024 17:59:14 GMT
< Date: Thu, 19 Dec 2024 17:59:14 GMT
< Connection: keep-alive
< Keep-Alive: timeout=5
< Transfer-Encoding: chunked
< 
0:[[],"$L1"]
1:[[],"$L2"]
2:["$","div",null,{"children":"Hello World 02"}]
* Connection #0 to host localhost left intact

What is very dangerous that any file called page.jsx in a folder is used for the directory index.

lazarv added a commit that referenced this issue Dec 23, 2024
Moves route matching and route component loading into middleware when
using the file-system based router to remove async implementation in the
React Server Component entrypoint of the router.

Changes RSC payload handling to use the same approach as in the HTML SSR
renderer to handle async components better. Introduces a rendering lock
mechanism to suspend sending the response to be able to handle response
headers in async components until the lock is released.

Refactors HTTP response header handling to use `Headers` directly and
introduces helper functions to set, append, delete, or clear response
headers in the HTTP context.

Fixes an issue with accessing the `Response` sent to the client when
using `useResponse` in an async React Server Component.

Adds test cases for above changes and updates the docs to include the
new helpers and to give more details about how to handle response
headers.

#100
@lazarv
Copy link
Owner

lazarv commented Dec 23, 2024

Thanks for all the details @aheissenberger!

This was a huge catch and an extremely important fix was needed to address this issue in the file-system based router entrypoint component, which was unnecessarily an async component, forcing the rendering to skip any response header changes in any React components when sending an RSC payload.

I also changed how to handle response headers and while it was quick and convenient to use a simple object to store the response headers, it was also an incomplete and poor solution.

Some more information is in the PR description at #106

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants