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

Cookies set in thrown responses are ignored when the CatchBoundary is in a parent route #3157

Closed
eranhirsch opened this issue May 11, 2022 · 6 comments · Fixed by #6431 or #6475
Closed
Labels

Comments

@eranhirsch
Copy link

eranhirsch commented May 11, 2022

What version of Remix are you using?

1.4.3

Steps to Reproduce

  1. Create a new remix app using npx create-remix and use the Just the basics and Remix App Server settings
  2. Create the parent layout ~/app/routes/parent.tsx:
export function CatchBoundary() {
  return <main>I'm a catch</main>;
}

export default function Parent() {
  return (
    <main>
      <header>Parent</header>
      <Outlet />
    </main>
  );
}
  1. Create the child ~/app/routes/parent/child.tsx:
export const loader: LoaderFunction = ({ request }) => {
  if (new URL(request.url).searchParams.has("throw")) {
    throw json(null, { headers: { "Set-Cookie": "thrownCookie=DoesntWork" } });
  }
  return json(null, { headers: { "Set-Cookie": "regularFlowCookie=ThisWorks" } });
};

export default function Child() {
  return <section>Child</section>;
}
  1. Run the server npm run dev
  2. curl --head "localhost:3000/parent/child"
  3. curl --head "localhost:3000/parent/child?throw"

Expected Behavior

The Set-Cookie header should be shown in both cases

Actual Behavior

When the response is thrown in a child of a CatchBoundary, Set-Cookie headers are not copied

@eranhirsch
Copy link
Author

eranhirsch commented May 11, 2022

This happens because of
https://github.com/remix-run/remix/blob/main/packages/remix-server-runtime/server.ts#L404
which filters out the modules below the CatchBoundary.

Because that filtered prefix of routes is the one used to generate the headers:
https://github.com/remix-run/remix/blob/main/packages/remix-server-runtime/server.ts#L434-L439

The problem is that, at this stage of the response logic, we don't track that a loader response was thrown or if it was returned in a happy flow (except logging the catch itself and the response data and status). So while it makes sense to skip the data (and headers) of child loaders that ran successfully, as they would be irrelevant because of the CatchBoundary at their parent, the thrown response should be treated as if it is "rendered" (it is what the CatchBoundary "catches") so it's headers should be handled too.

@brophdawg11
Copy link
Contributor

Fixed by #6475 and will be available in the next release (likely 1.17.0)

@brophdawg11 brophdawg11 added the awaiting release This issue has been fixed and will be released soon label May 24, 2023
@brophdawg11 brophdawg11 removed their assignment May 24, 2023
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-33cc4c0-20230525 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@eranhirsch
Copy link
Author

wow, it's been a year! thanks for circling back to the task to update! Waiting for the next release to remove my patched solution

@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2023

🤖 Hello there,

We just published version 1.17.0-pre.0 which involves this issue. 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
Copy link
Contributor

github-actions bot commented Jun 6, 2023

🤖 Hello there,

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

Thanks!

@brophdawg11 brophdawg11 removed the awaiting release This issue has been fixed and will be released soon label Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants