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

Fixes #86 - Pluggable Fetch #109

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ All the retry options are configurable and can be set in `retryOptions` in the `
| `retryOnHttpError` | Function | a *function* determining whether to retry given the HTTP error exception thrown. Can be asynchronous | none | retry on all `FetchError`'s of type `system`|
| `socketTimeout` | Number | time until socket timeout in milliseconds. _Note: if `socketTimeout` is >= `retryMaxDuration`, it will automatically adjust the socket timeout to be exactly half of the `retryMaxDuration`. To disable this feature, see `forceSocketTimeout` below_ | `NODE_FETCH_RETRY_SOCKET_TIMEOUT` | 30000 ms |
| `forceSocketTimeout` | Boolean | If true, socket timeout will be forced to use `socketTimeout` property declared regardless of the `retryMaxDuration`. _Note: this feature was designed to help with unit testing and is not intended to be used in practice_ | `NODE_FETCH_RETRY_FORCE_TIMEOUT` | false |
| `fetch` | Function | the fetch API implementation to use | none |
| `AbortController` | Function | the AbortController constructor function to use | none |

_Note: the environment variables override the default values if the corresponding parameter is not set. These are designed to help with unit testing. Passed in parameters will still override the environment variables_

Expand Down Expand Up @@ -162,6 +164,23 @@ async main() {
}
```

### Custom Fetch

You can disable all retry behavior by setting `retryOptions` to `false`.

```js
const {fetch: adobeFetch, AbortController} = require('@adobe/fetch');
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the difference between adobe/fetch's abort controller and ours?

const fetch = require('@adobe/node-fetch-retry');

async main() {
const response = await fetch(url, {
retryOptions: false,
fetch: adobeFetch,
AbortController
});
}
```

Disabling retry behavior will not prevent the usage of other options set on the `options` object.

### Additional notes on retry duration
Expand Down
7 changes: 5 additions & 2 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ function shouldRetryOnHttpError(error) {
/**
* @typedef {Object} Options options for fetch-retry
* @property {Object} RetryOptions options for retry or false if want to disable retry
* @property {Function} fetch the fetch implementation to use
* ... other options for fetch call (method, headers, etc...)
*/
/**
Expand All @@ -209,13 +210,15 @@ module.exports = async function (url, options) {

let timeoutHandler;
if (retryOptions.socketTimeout) {
const controller = new AbortController();
const abortCon = options.AbortController || AbortController;
const controller = new abortCon();
timeoutHandler = setTimeout(() => controller.abort(), retryOptions.socketTimeout);
options.signal = controller.signal;
}

try {
const response = await fetch(url, options);
const fetchFn = options.fetch || fetch;
Copy link
Contributor

Choose a reason for hiding this comment

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

this recursively invokes wrappedFetch(); so i would prefer for you to set the fetchFn once if possible. (somehow globally maybe).
Maybe there is a way to declare on import which fetch to use?

In case the options may get wiped away between retries (maybe this is too unrealistic)

const response = await fetchFn(url, options);

if (await shouldRetry(retryOptions, null, response, waitTime)) {
console.error(`Retrying in ${waitTime} milliseconds, attempt ${attempt} failed (status ${response.status}): ${response.statusText}`);
Expand Down
Loading