Skip to content

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

Closed
mcansh wants to merge 5 commits into
devfrom
logan/rem-1404-returned-response-from-fetch
Closed

fix(remix-node): convert fetch's WebResponse to NodeResponse so it matches the global#4148
mcansh wants to merge 5 commits into
devfrom
logan/rem-1404-returned-response-from-fetch

Conversation

@mcansh

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

Copy link
Copy Markdown
Contributor

closes #4395, #3480

@changeset-bot

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

Copy link
Copy Markdown

🦋 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
MichaelDeBoey
MichaelDeBoey approved these changes Sep 29, 2022
View reviewed changes
Comment thread .changeset/late-worms-impress.md
@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

MichaelDeBoey commented Jan 24, 2023

Copy link
Copy Markdown
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

acoreyj commented Jan 24, 2023

Copy link
Copy Markdown

@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

cliffordfajardo commented Feb 18, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

@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

MichaelDeBoey commented Mar 6, 2023

Copy link
Copy Markdown
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

MichaelDeBoey commented May 1, 2023

Copy link
Copy Markdown
Member

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

@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
@PtrkSki

PtrkSki commented May 20, 2023

Copy link
Copy Markdown

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
Comment thread packages/remix-node/fetch.ts
};

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

@kiliman kiliman May 20, 2023

Copy link
Copy Markdown
Contributor

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 added 4 commits June 13, 2023 23:20
@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

jacob-ebey commented Aug 9, 2023

Copy link
Copy Markdown
Member

Should be closed by #7109

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

github-actions Bot commented Aug 9, 2023

Copy link
Copy Markdown
Contributor

🤖 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

github-actions Bot commented Aug 10, 2023

Copy link
Copy Markdown
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

@kiliman kiliman kiliman left review comments

@MichaelDeBoey MichaelDeBoey MichaelDeBoey approved these changes

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

Assignees

@jacob-ebey jacob-ebey

Labels

CLA Signed

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 @PtrkSki @jacob-ebey @kiliman @brophdawg11