fix(react-router/server-runtime): delay turbo-stream serialization of .data redirects

[brophdawg11]

This dedupes a good bit of code and keeps the middleware chain a bit more consistent between document/data requests since you can now inspect and "see" a redirect in both cases and you don't have to be aware of the implementation detail that is the turbo-stream encoding and the 202 response. We just wait until right before we send the response before we serialize it via turbo-stream.

function middleware() {
  let res = await next();

  // Currently, `res` would be a 202 response on `.data` requests and you can't easily 
  // see the location because it's encoded in the body.

  // Now, it's a standard 3xx redirect with a `Location` header on both document 
  // and data requests

  return res;
}

function loader() {
  return redirect('/');
}

[changeset-bot]

🦋 Changeset detected

Latest commit: 71c1f8c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 11 packages

Name	Type
react-router	Patch
@react-router/architect	Patch
@react-router/cloudflare	Patch
@react-router/dev	Patch
react-router-dom	Patch
@react-router/express	Patch
@react-router/node	Patch
@react-router/serve	Patch
@react-router/fs-routes	Patch
@react-router/remix-routes-option-adapter	Patch
create-react-router	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

[github-actions]

🤖 Hello there,

We just published version 7.8.2-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

[github-actions]

🤖 Hello there,

We just published version 7.8.2 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

[aaronadamsCA]

@brophdawg11, I think this change may have unintentionally had the opposite effect on our Playwright integration tests.

We have route actions that throw redirects in specific cases. Until now, tests for those cases could just examine a response object's Location header.

As of 7.8.2, it looks like those tests would instead "have to be aware of the implementation detail that is the turbo-stream encoding and the 202 response".

I have no idea what to do with this finding, what do you think? Should I file a bug, a request for test tools, or something else entirely?

[brophdawg11]

@aaronadamsCA I think I would need to see an example. This shouldn't change anything about how redirects are returned on document or data requests:

• document requests should return normal 3xx redirect responses with a Location header
• data requests should still return 202 responses with the redirect info encoded into the body via turbo-stream

What should change here is the point in time when redirects on .data requests are converted from a 3xx to a 202 when middleware is enabled. You used to get a 202 from await next() on .data requests in middleware, but now you'll get the standard 3xx and it'll be converted to a 202 as the very last step before being returns from the react router handler.

Here's a quick example: https://stackblitz.com/edit/github-5cdfffvj?file=app%2Froutes%2Fredirect.tsx

If you downgrade that to 7.8.1 you can see the old 202 behavior.

[aaronadamsCA]

Makes sense, @brophdawg11, and I see what's happening now.

I think the only thing it revealed is that our testing approach was all wrong.

Prior to 7.8.2 the 202 response had a Location header. Now you're removing it. We were unwittingly relying on that header with this test, which was written to assume it was inspecting a 3XX response:

test("redirects on POST request (old approach)", async ({ page }) => {
  await page.goto("http://localhost:5173/");

  const [request] = await Promise.all([
    page.waitForRequest((request) => request.method() === "POST"),
    page.getByRole("button").click(),
  ]);

  const response = await request.response();
  assert.ok(response != null);

  const location = await response.headerValue("location");
  expect(location).toStrictEqual(URL);
});

But examining a response like that is essentially examining a React Router internal. How you implement the redirect is none of our concern.

What we probably should have done all along, and will do going forward, is to examine the actual redirect request:

test("redirects on POST request (new approach)", async ({ page }) => {
  await page.goto("http://localhost:5173/");

  const [request] = await Promise.all([
    page.waitForRequest((request) => request.resourceType() === "document"),
    page.getByRole("button").click(),
  ]);

  expect(request.url()).toStrictEqual(URL);
});

This works across versions, hydrated or not.

[brophdawg11]

ahh ok yeah that makes sense. Those 202 responses weren't supposed to have the Location header anymore once it was serialized into the body but our prior approach was incorrectly preserving it.

Your new approach is the right way to validate - just check the result of the redirect in the browser 👍