-
Notifications
You must be signed in to change notification settings - Fork 224
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
feat(fetch-requester): add @algolia/requester-fetch
#1411
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit cbebf59:
|
First of all, thanks for this thorough pull request. It looks good on first glance, but I'll make sure to test it completely next week. It makes sense for the future (even across major) to keep xhr as the default (as you've done here), as there's some other environments that don't have fetch (ie, react native) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks great and works as expected (sandbox that uses it: https://codesandbox.io/s/javascript-client-app-forked-iwbbje?file=/src/app.js). Some small nitpicks are all I have to add
packages/requester-fetch/src/__tests__/unit/fetch-requester.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing to add, thanks a lot for the contribution!
package.json
Outdated
@@ -50,6 +50,7 @@ | |||
"@wdio/static-server-service": "5.16.10", | |||
"barrelsby": "2.2.0", | |||
"bundlesize": "0.18.0", | |||
"cross-fetch": "^3.1.5", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"cross-fetch": "^3.1.5", | |
"cross-fetch": "3.1.5", |
We can let renovate do the things
Thanks for this! Not sure if I'm missing something but unfortunately CloudFlare does not implement all parameters of the fetch api making it fails when I tried using it. I get a RetryError with the following: The 'mode' field on 'RequestInitializerDict' is not implemented If I comment out https://github.com/algolia/algoliasearch-client-javascript/blob/4.14.3/packages/requester-fetch/src/createFetchRequester.ts#L47 then it works. Npm deps:
Run locally: npx wrangler dev src/index.js
|
Hey @my2ter! we've change the order of the parameters in v5 (https://github.com/algolia/algoliasearch-client-javascript/blob/next/packages/requester-fetch/src/createFetchRequester.ts#L39-L49), which allows the user to override the value for the If you wish to open the PR with the changes, please feel free! Otherwise I'll try to do it tomorrow |
Edge Computing such as Cloudflare Worker and Edge Functions (Vercel) do not support XHR or Node.js HTTP modules. add a requester that uses Fetch and Edge Computing. Algolia's JS client can be used with Edge Computing by adding a requester that uses Fetch.