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

Fix native fetch immutable headers issue #9693

Merged
merged 5 commits into from
Jul 3, 2024

Conversation

brophdawg11
Copy link
Contributor

Fixes #9691

Copy link

changeset-bot bot commented Jul 2, 2024

🦋 Changeset detected

Latest commit: 9d6bbf9

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

This PR includes changesets to release 16 packages
Name Type
@remix-run/server-runtime Patch
@remix-run/cloudflare Patch
@remix-run/deno Patch
@remix-run/dev Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/testing Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/architect Patch
@remix-run/express Patch
@remix-run/serve Patch
create-remix Patch
remix Patch
@remix-run/css-bundle Patch
@remix-run/eslint-config 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

@brophdawg11 brophdawg11 linked an issue Jul 2, 2024 that may be closed by this pull request
parentId: "root",
index: true,
loader() {
return fetch(`https://remix.run/docs/en/main?_data=root`);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really want to keep this here but the only way to replicate this issue was to actually get a legit Response from a fetch call which is why we didn't run into this before. Thoughts on how to best prevent a regression in this area without spamming our prod server?

@@ -360,12 +360,12 @@ async function handleDataRequest(

// Mark all successful responses with a header so we can identify in-flight
// network errors that are missing this header
response.headers.set("X-Remix-Response", "yes");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this response is a direct undici fetch Response, then the headers are immutable and we can't change them. I didn't want to clone the response in 100% of scenarios since this isn't an issue with return json() and alternatives, so I am only cloning if this operation throws the immutable error

@brophdawg11 brophdawg11 merged commit d6bc2ba into dev Jul 3, 2024
5 checks passed
@brophdawg11 brophdawg11 deleted the brophdawg11/fix-native-fetch-headers branch July 3, 2024 13:40
@github-actions github-actions bot added the awaiting release This issue has been fixed and will be released soon label Jul 3, 2024
Copy link
Contributor

github-actions bot commented Jul 4, 2024

🤖 Hello there,

We just published version 2.10.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!

Copy link
Contributor

github-actions bot commented Jul 4, 2024

🤖 Hello there,

We just published version 2.10.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!

@github-actions github-actions bot removed the awaiting release This issue has been fixed and will be released soon label Jul 4, 2024
@myfrontpocket
Copy link

myfrontpocket commented Jul 9, 2024

So, I think this is the same issue here: #9212

I've updated to 2.10.2, and this does restore the previous behavior, with the exception that returning the error response results in a framework exception (that I suppose would be caught by an ErrorBoundary). Shouldn't this only happen if we throw the response rather than return it?

const response = await fetch('some-url.com')

if (!response.ok) {
  return response; // this acts a throw now
}

// continue processing...

This code before would return as loader/action data, but now it does not 😢

@brophdawg11
Copy link
Contributor Author

I do not see that behavior. Please open a ne wissue with a reporduction and we can take a look. But my quick attempt works as expected:

// routes/page.tsx
import { useLoaderData } from "@remix-run/react";

export async function loader() {
  const response = await fetch("http://localhost:5173/resource");

  if (!response.ok) {
    return response;
  }

  return { message: "from loader" };
}

export default function Comp() {
  const data = useLoaderData();
  return <p>data: {data?.message}</p>;
}

// routes/resource.tyx
import { json } from "@remix-run/node";

export function loader() {
  return json({ message: "from resource route" }, { status: 500 });
}

@myfrontpocket
Copy link

myfrontpocket commented Jul 9, 2024

I do not see that behavior. Please open a ne wissue with a reporduction and we can take a look. But my quick attempt works as expected:

// routes/page.tsx
import { useLoaderData } from "@remix-run/react";

export async function loader() {
  const response = await fetch("http://localhost:5173/resource");

  if (!response.ok) {
    return response;
  }

  return { message: "from loader" };
}

export default function Comp() {
  const data = useLoaderData();
  return <p>data: {data?.message}</p>;
}

// routes/resource.tyx
import { json } from "@remix-run/node";

export function loader() {
  return json({ message: "from resource route" }, { status: 500 });
}

Hmmm... is it the same for an action function? Not sure if it'll make a difference. This is where I'm currently seeing it happen

Nevermind, it is in a loader where I'm seeing this behavior. I'll investigate my code further and see if I can figure it out. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Loader error on data request proxying raw fetch response
3 participants