search

lazarv / react-server

The easiest way to build React apps with server-side rendering
https://react-server.dev
MIT License
121 stars 6 forks source link

fix: accept header `*/*` is a valid request #49

Closed aheissenberger closed 2 months ago

aheissenberger commented 2 months ago

The ssr handler should return a valid result if the accept http header is or contains */*.

lazarv commented 2 months ago

Hi @aheissenberger! I have some issues with this change.

At the point of change, the condition is used for early-exit from the SSR handler just before trying to render a component in HTML, RSC payload (text/x-component) or handling server actions. When using the Hattip handlers return resolve() means that we are resolving the promise we are within at that point, letting the handlers to continue with next available handler, which were defined here: https://github.com/lazarv/react-server/blob/main/packages/react-server/lib/start/create-server.mjs#L75 and if the Accept header is incorrect for SSR rendering, it follows with post handlers provided by the developer if there were any and the last handler just return a 404 response.

In the case of framework middlewares (defined using init$, more about this at https://react-server.dev/router/server-routing#middlewares) the SSR handler process these before checking the Accept header and so allowing any kind of requests to be processed by these middlewares at https://github.com/lazarv/react-server/blob/main/packages/react-server/lib/start/ssr-handler.mjs#L179

The last exit point from the SSR handler is when a middleware used redirect, which sets a state in an async storage and this is implemented at https://github.com/lazarv/react-server/blob/main/packages/react-server/lib/start/ssr-handler.mjs#L185

Furthermore, even the rendering is checking the Accept header to choose the proper rendering path at https://github.com/lazarv/react-server/blob/main/packages/react-server/server/render-rsc.jsx#L269 and https://github.com/lazarv/react-server/blob/main/packages/react-server/server/render-rsc.jsx#L387, and also making some tweaks on rendering based on the Accept header, like selecting the proper cache type or enabling RSC delegation.

Could you please provide more info on the intention of your change? Thanks!

aheissenberger commented 2 months ago

I do understand the reason for this, but this is the first framework I use with this kind of restriction. The problem I had is that in my default settings, AWS Cloudfront is not forwarding the accept header, as it makes caching of request more difficult as this header varies with different browsers for the same request.

It was no problem until now with RSC frameworks (e.g. WAKU, Vinxi) or fullstack e.g Vike.dev. Have you thought about using a custom header as used by some other frameworks to distinguish RSC requests as much smaller data marker?

It's, small problem and it was easy to solver. I would understand if you would currently not change this behavior.

lazarv commented 2 months ago

Have you thought about using a custom header as used by some other frameworks to distinguish RSC requests as much smaller data marker?

Yes, that would be a much better way to drive the rendering logic, thanks for the feedback and suggestion on this!