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

tests: always use node-fetch to simulate browser fetch #437

Merged
merged 1 commit into from
Aug 30, 2023

Conversation

pablopalacios
Copy link
Contributor

Since httpRequest is only used in the browser, it doesn't matter much if we are using node-fetch, isomorphic-fetch, undici or node's native fetch. Said that, to avoid tests to fail on Node 18 (which includes a native fetch implementation), we can overwrite the global fetch with node-fetch's implementation.

I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

Since httpRequest is only used in the browser, it doesn't matter much
if we are using node-fetch, isomorphic-fetch, undici or node's native
fetch. Said that, to avoid tests to fail on Node 18 (which includes a
native fetch implementation), we can overwrite the global fetch with
node-fetch's implementation.
@pablopalacios
Copy link
Contributor Author

By the way, the tests seem to fail because of an undici issue: cloudflare/miniflare#454. In the browser, new Request('/api') works fine, but in Node 18, it throws an invalid URL error since the host is missing. According to fetch spec, the browser behavior is correct, but it's also understandable that it wouldn't work in Node since there is no base uri set.
2023-08-30-233540_599x166_scrot

Copy link
Contributor

@redonkulus redonkulus left a comment

Choose a reason for hiding this comment

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

Thank you for the quick turnaround!

Interesting fix, makes sense to me. I was looking into the Request not having the host but was getting tripped up since there wasn't one in Node. It would be nice long term to use the built in fetch but I understand for testing a client only library, that this is necessary.

@redonkulus redonkulus merged commit b6ece76 into yahoo:master Aug 30, 2023
2 checks passed
@pablopalacios pablopalacios deleted the fix-tests branch August 31, 2023 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants