-
Notifications
You must be signed in to change notification settings - Fork 90
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
problematic tests regarding Node.js built-in fetch
#1612
Comments
About the network error test, the code that we're testing here is flawed. As I said, it's not entirely standardized, so this doesn't work well across browsers and runtimes. https://github.com/meilisearch/meilisearch-js/blob/main/src/errors/meilisearch-communication-error.ts this probably only works well for On built-in Node.js Also |
While the tests are passing now because of #1622, it is still relevant, because we're messing with non-standard properties of |
Okay, so the problem is that builtin The solution is to pass a random truly custom header, like |
Newer versions of Node.js (LTS 20.10.0, Maintenance 18.19.0) have built-in
fetch
, so thecross-fetch
polyfill has no effect. This means the built-infetch
is used, but this presents a problem with the tests that are run:So far I identified two reoccurring errors:
The problem is that network and other errors aren't really standardized in
fetch
.There are two potential paths we can take to address this problem:
fetch
andnode-fetch
fetch
support, so we only need to test thatrelated: #1610
The text was updated successfully, but these errors were encountered: