Simplify renderHydrogen internal logic

[frandiox]

Description

I need to separate RSC from SSR logic for a new RSC<>Vite approach. While doing that, I thought it could be helpful to extract part of the refactoring in this PR.

We had 3 functions that ran RSC independently, and 2 of them did SSR as well: hydrate, render and stream. This PR reduces this to runRSC and runSSR.
For bots, runSSR will buffer the response and transform the HTML. This also applies now when the user calls response.doNotStream().

I thought I had introduced a bug but it seems we already have it in main branch.

Is there anything I've missed? 🤔

Additional context

It might be easier to check the commits 1 by 1.

---

Before submitting the PR, please make sure you do the following:

• Read the Contributing Guidelines
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123)
• Update docs in this repository according to your change
• Run yarn changeset add if this PR cause a version bump based on Keep a Changelog and adheres to Semantic Versioning

[github-actions]

We detected some changes in packages/*/package.json or packages/*/src, and there are no updates in the .changeset.
If the changes are user-facing and should cause a version bump, run "yarn changeset add" to track your changes and include them in the next release CHANGELOG.
If you are making simple updates to examples or documentation, you do not need to add a changeset.

[wizardlyhel]

I ran thru with Cloudflare worker with streaming turn on.

export async function isStreamingSupported() {
  if (globalThis.Oxygen) {
    return Boolean(globalThis.Oxygen?.env?.HYDROGEN_ENABLE_WORKER_STREAMING);
  }
  return true;
}

Tested with:

• Full page navigation
• RSC navigation
• Api route (sitemap.xml)
• Disable streaming with ?_bot

Did I miss anything?

[blittle]

@wizardlyhel that sounds about right. We have automated tests for most other things, XSS, form vs json requests in API routes, etc

[blittle]

I love the idea of this change. I think it's simpler and makes more sense. Each request is either an RSC or SSR request. Both of which could be streamed or buffered.

[blittle]

@frandiox I've done some more testing and it looks like these changes break the auth login. A few issues:

1. On workers, the logout API route doesn't seem to be propagating the set-cookie response header. In node it works fine.
2. I'm unable to login, I'm seeing a 500 error response from the server, but no actual error message or stack trace. Besides the obvious issue that login doesn't work, also it looks like errors aren't getting caught or logged properly. See the screenshot:

[blittle]

The issues I listed are actually in mini-oxygen itself. I sent a few PRs to resolve them. As a result, I think this looks good :)

[jplhomer]

Terrific work 👏

[jplhomer]

Let's update this variable name while we're at it? isRSCRequest maybe

[jplhomer]

Curious what rsc.startsWith check is here. Why would the flight response ever start out with an HTML tag?

[frandiox]

Ahh, I was supposed to remove that!
I had a brainfart thinking at this point, rsc could already be in containers if this was streamable but the user called doNotStream manually. But no, at this point that doesn't happen.

[jplhomer]

ha way more elegant than my last attempt 😂

[frandiox]

@blittle I wonder if these changes now make config#enableStreaming obsolete?
I think one could just do the same thing in App.server.jsx:

export default function App({request, response}) {
  if (request.headers.get('user-agent') === 'custom bot') response.doNotStream();

  return <>...</>
}

Hydrogen would now format the output HTML properly like we used to do in render.

Edit: New PR for this Shopify/hydrogen#1401