-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Bug reproduction: web fetch fails on https://gmail.com and crashes entire server #3404
Conversation
Hi @vedantroy, Welcome, and thank you for contributing to Remix! Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once. You may review the CLA and sign it by adding your name to contributors.yml. Once the CLA is signed, the If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run. Thanks! - The Remix team |
I'm seeing this error occasionally when I am trying to use fetch and get 301 responses. My fetch is already wrapped in a try/catch so I'm not sure how to properly catch this error.
|
Downgrading to |
Probably happened when we moved from node-fetch to @remix-run/web-fetch in #2736 |
This has bugs but is allowing me to work around the issue in my specific case. const fetchWithManualRedirect = (url, opts) => fetch(url, opts = {}).then(res => {
return [301, 302].includes(res.status) ?
fetchWithManualRedirect(res.headers.get('location'), {...opts, redirect: 'manual'})
: res
}) The bug appears to be within the fetch package, specifically around how the redirect options function. |
Ran into the same issue a couple months back and the same workaround worked for me (i.e. don't automatically follow redirects). In my case, the server in question was sending a redirect response w/ chunked transfer encoding. I think this might be the commit that needs to be backported (but it might not be trivial since afaik @remix-run/web-fetch is based on 2.x): node-fetch/node-fetch#1222 |
I am also running into this same error now when requesting a pdf resource with fetch. Using |
Oh, @jacob-ebey, I didn't mean for this issue to actually be closed 🤦♂️. The integration of the updated fetch package with remix will fix this one. Sorry, I thought my phrasing in my PR wouldn't have triggered github's auto-close feature. |
still having the error after upgrading the version to 1.6.5 |
@raymclee 1.6.5 was released before this got merged. It will be shipped in the next release. /cc @jacob-ebey for confirmation of both my claims |
got it! thanks |
My concern is that the web-fetch package failed to release and wasn't brought into remix. A release today would not fix the issue. |
Re-opening for visibility |
@mcansh just got the web-fetch release cut, so if you delete package-lock.json and your node modules folder, you'll pull in |
This also appears to work: npm up @remix-run/web-fetch@latest --no-package-lock |
Hi @BenMcH i know this seems to be fixed in below are some dependencies;
|
Having the same issue with |
+1 also having this issue On the first route of the first project I'm trying with remix so not a really good look 😓 The PR looks good to me, would be great to get an approval! |
When we run
await fetch("https://gmail.com")
, in a loader, we get this error:On its own, this is no massive issue. The problem is that the server crashes after this, rendering it incapable of handling new requests.