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.

Sign up for GitHub

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

Jump to bottom

fix(remix-node): convert fetch's WebResponse to NodeResponse so it matches the global #4148

Closed
wants to merge 5 commits into from
Closed

fix(remix-node): convert fetch's WebResponse to NodeResponse so it matches the global #4148

wants to merge 5 commits into from
+52 −2

Conversation

mcansh
Copy link
Collaborator

@mcansh mcansh commented Sep 6, 2022 •
edited by brophdawg11
Loading

closes #4395, #3480

@changeset-bot
Copy link

changeset-bot bot commented Sep 6, 2022 •
edited
Loading

🦋 Changeset detected

Latest commit: 426370c

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

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

@mcansh mcansh force-pushed the logan/rem-1404-returned-response-from-fetch branch from 967a676 to 2132041 Compare September 6, 2022 16:53
@MichaelDeBoey MichaelDeBoey changed the title fix(node): convert fetch WebResponse to NodeResponse so it matches the global fix(remix-node): convert fetch's WebResponse to NodeResponse so it matches the global Sep 29, 2022
@mcansh mcansh force-pushed the logan/rem-1404-returned-response-from-fetch branch from 66791c4 to 2f91106 Compare December 7, 2022 20:28
@MichaelDeBoey MichaelDeBoey mentioned this pull request Jan 24, 2023
Closed
1 task
@MichaelDeBoey
Copy link
Member

@acoreyj It seems like this is doing the same thing as your implementation in #5095 or am I missing some things?

@MichaelDeBoey MichaelDeBoey force-pushed the logan/rem-1404-returned-response-from-fetch branch from 2f91106 to 71db7d2 Compare January 24, 2023 14:20
@MichaelDeBoey MichaelDeBoey mentioned this pull request Jan 24, 2023
Closed
2 tasks
@acoreyj
Copy link

acoreyj commented Jan 24, 2023

@acoreyj It seems like this is doing the same thing as your implementation in #5095 or am I missing some things?

@MichaelDeBoey Ah wow I apparently did a bad job searching PRs, o well now I know a lot more about fetch.

Yes this is the same, and looking through the webfetch code it always resolves a webResponse and never rejects one so my extra checks and catch aren't necessary.

Plus the test strategy is definitely better

I think you are good to merge this and close mine.

@cliffordfajardo
Copy link
Contributor

cliffordfajardo commented Feb 18, 2023 •
edited
Loading

@mcansh following on this since thus PR seems this PR in the auth.js for adding remix support for the auth.js lib seems to be dependent on this PR getting merged 🙏

nextauthjs/next-auth#6270

@acoreyj acoreyj mentioned this pull request Feb 20, 2023
Open
3 tasks
@MichaelDeBoey MichaelDeBoey force-pushed the logan/rem-1404-returned-response-from-fetch branch from 71db7d2 to 5000979 Compare March 6, 2023 20:32
@MichaelDeBoey
Copy link
Member

@mcansh I think this one can be merged as-is or is there anything that we're still waiting for?

@MichaelDeBoey MichaelDeBoey force-pushed the logan/rem-1404-returned-response-from-fetch branch from 5000979 to 4c12784 Compare March 9, 2023 00:17
@MichaelDeBoey MichaelDeBoey force-pushed the logan/rem-1404-returned-response-from-fetch branch from 4c12784 to 8ab7cc1 Compare May 1, 2023 23:21
@MichaelDeBoey
Copy link
Member

@mcansh I don't think anything is holding us back from merging this one?

@brophdawg11 brophdawg11 added the feat:fetch Issues related to @remix-run/web-fetch label May 5, 2023
@brophdawg11 brophdawg11 linked an issue May 5, 2023 that may be closed by this pull request
fetch() doesn't return correct Response type #4395
Closed
@thedotfilesdev
Copy link

Any chances that this one would be merged in the near future 🙏🏻 . Would be great to be able to use @auth/core together with the remix.

kiliman
kiliman reviewed May 20, 2023
View reviewed changes
packages/remix-node/fetch.ts
@@ -72,5 +72,7 @@ export const fetch: typeof webFetch = (
...init,
};

return webFetch(info, init as RequestInit);
let webResponse = await webFetch(info, init as RequestInit);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't this change the expectation of fetch, since now you await the response before returning? What if the original response is streaming?

@mcansh @MichaelDeBoey
fix(node): convert fetch WebResponse to NodeResponse so it matches th…
389d887 
…e global

Signed-off-by: Logan McAnsh <logan@mcan.sh>
@mcansh @MichaelDeBoey
Create late-worms-impress.md
57f9c66
@mcansh @MichaelDeBoey
chire: rename test
65a7db1 
Signed-off-by: Logan McAnsh <logan@mcan.sh>
@mcansh @MichaelDeBoey
chore: fix test file name
c769fbe 
Signed-off-by: Logan McAnsh <logan@mcan.sh>
@MichaelDeBoey MichaelDeBoey force-pushed the logan/rem-1404-returned-response-from-fetch branch from 8ab7cc1 to c769fbe Compare June 13, 2023 21:20
@brophdawg11 brophdawg11 removed their request for review June 14, 2023 14:19
This was referenced Aug 8, 2023
Closed
Closed
Merged
@jacob-ebey
Copy link
Member

Should be closed by #7109

@jacob-ebey jacob-ebey closed this Aug 9, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2023

🤖 Hello there,

We just published version v0.0.0-nightly-a179aa7-20230809 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

🤖 Hello there,

We just published version v0.0.0-nightly-b1149bb-20230810 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!

@MichaelDeBoey MichaelDeBoey deleted the logan/rem-1404-returned-response-from-fetch branch August 15, 2023 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichaelDeBoey MichaelDeBoey MichaelDeBoey approved these changes

@kiliman kiliman kiliman left review comments

@jacob-ebey jacob-ebey Awaiting requested review from jacob-ebey

Assignees

@jacob-ebey jacob-ebey

Labels
CLA Signed feat:fetch Issues related to @remix-run/web-fetch runtime:node
Projects
No open projects
v2
Status: Closed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fetch() doesn't return correct Response type
8 participants
@mcansh @MichaelDeBoey @acoreyj @cliffordfajardo @thedotfilesdev @jacob-ebey @kiliman @brophdawg11