Avoid passing undefined through to async-retry, remove maxTries option #306
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The makeRequest function accepts some options that default to undefined
when they are not specified. These are then passed into async-retry,
which eventually gets merged into the default options in a loop like
this:
```js
for (var key in options) {
opts[key] = options[key];
}
```
https://github.com/tim-kos/node-retry/blob/11efd6e4/lib/retry.js#L17-L26
This means that when the options are not specified, they end up
overriding the reasonable defaults, which was unintentional.
The result is that since we don't specify these options most of the
time, most of the requests will be retried immediately instead of
waiting a little bit.
To fix this, we need to construct the options a little more carefully.
I wanted to test this using jest fake timers, but when I did that it
caused the tests to stall out and I wasn't able to figure out why in a
reasonable amount of time, so instead I decided that it was okay for the
test to take a second and just set the number of retries to a low
amount.
While I am touching this, I decided to also simplify the API a small
amount by removing a really old leacy `maxTries` option.
The signed variable doesn't change between retries, so we can just generate it once instead of on each retry. This makes the code a bit more efficient.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The makeRequest function accepts some options that default to undefined when they are not specified. These are then passed into async-retry, which eventually gets merged into the default options in a loop like this:
https://github.com/tim-kos/node-retry/blob/11efd6e4/lib/retry.js#L17-L26
This means that when the options are not specified, they end up overriding the reasonable defaults, which was unintentional.
The result is that since we don't specify these options most of the time, most of the requests will be retried immediately instead of waiting a little bit.
To fix this, we need to construct the options a little more carefully.
I wanted to test this using jest fake timers, but when I did that it caused the tests to stall out and I wasn't able to figure out why in a reasonable amount of time, so instead I decided that it was okay for the test to take a second and just set the number of retries to a low amount.
While I am touching this, I decided to also simplify the API a small amount by removing a really old leacy
maxTriesoption.