Skip to content

fix(remix-node): make global fetch return an instance of global Response#5095

Closed
acoreyj wants to merge 8 commits into
remix-run:mainfrom
acoreyj:fix-4395
Closed

fix(remix-node): make global fetch return an instance of global Response#5095
acoreyj wants to merge 8 commits into
remix-run:mainfrom
acoreyj:fix-4395

Conversation

@acoreyj

@acoreyj acoreyj commented Jan 13, 2023

Copy link
Copy Markdown

Closes: #4395

  • Docs
  • [ x] Tests

Testing Strategy:

This test covers this code: packages/remix-node/tests/fetch-test.ts it("should return an instance of Response and WebResponse"

@changeset-bot

changeset-bot Bot commented Jan 13, 2023

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 7e630a5

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@remix-cla-bot

remix-cla-bot Bot commented Jan 13, 2023

Copy link
Copy Markdown
Contributor

Hi @acoreyj,

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 CLA Signed label will be added to the pull request.

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

@remix-cla-bot

remix-cla-bot Bot commented Jan 13, 2023

Copy link
Copy Markdown
Contributor

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@acoreyj acoreyj changed the title Failing test for #4395 Fix for #4395 global fetch doesn't return global Response in remix-node Jan 13, 2023
@acoreyj

acoreyj commented Jan 17, 2023

Copy link
Copy Markdown
Author

Note that this blocks being able to use @authjs in remix node environments

nextauthjs/next-auth#6270

@MichaelDeBoey MichaelDeBoey changed the title Fix for #4395 global fetch doesn't return global Response in remix-node fix(remix-node): make global fetch return an instance of global Response Jan 22, 2023
@acoreyj

acoreyj commented Jan 23, 2023

Copy link
Copy Markdown
Author

Hmm I'm struggling to think of ways to mock this test to work in CI as mocking the fetch obviously ruins the point, any thoughts @MichaelDeBoey @jacob-ebey

@acoreyj

acoreyj commented Jan 24, 2023

Copy link
Copy Markdown
Author

@MichaelDeBoey I pulled out the logic for creating the response so I could mock the fetch itself and test this which should pass in the CI now

Comment on lines +82 to +87
.then((res) => {
return getReturnableResponse(res);
})
.catch((res) => {
return getReturnableResponse(res);
});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This could have a smaller footprint if we do

Suggested change
.then((res) => {
return getReturnableResponse(res);
})
.catch((res) => {
return getReturnableResponse(res);
});
.then(getReturnableResponse)
.catch(getReturnableResponse);

@MichaelDeBoey

Copy link
Copy Markdown
Member

Closing this as it seems to be a duplicate of @mcansh's #4148

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

Labels

CLA Signed duplicate This issue or pull request already exists

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fetch() doesn't return correct Response type

3 participants