Add ability to stop retries in beforeRetry hooks
#198
Conversation
index.js
Outdated
| this._options, | ||
| error, | ||
| this._retryCount, | ||
| beforeRetryResults.add( |
There was a problem hiding this comment.
It would be more consistent with the behavior of our other hooks if returning the symbol short-circuited the loop. Meaning, return immediately inside of the loop, preventing any further beforeRetry hooks from running.
That also removes the need to use a Set.
There was a problem hiding this comment.
I wasn't sure about that, my reasoning was that you'd want to have all your hooks running before making the "cancel retry" decision.
There was a problem hiding this comment.
I get what you are saying. I do see how this case is a bit different, as the symbol is not a true "value" being returned by the hook, unlike the other cases where a hook may return a Request or Response instance.
Still, I think it would be surprising for beforeRetry to behave differently in this respect than the other hook types. IMO, we need to have a holistic approach on whether returning a value from a hook short-circuits the chain. And we should apply that logic consistently.
|
It also needs to be documented in the readme. |
sindresorhus
changed the title
beforeRetry hooks
|
@sholladay Looks good to you? |
index.d.ts
Outdated
|
|
||
| @example | ||
| ``` | ||
| import ky, {stop as kyStop} from 'ky'; |
There was a problem hiding this comment.
I'd name it RETRY_STOP.
There was a problem hiding this comment.
We went with something generic so we can use the symbol for other places we want to support stopping.
There was a problem hiding this comment.
Okay, then it should be SYMBOL_STOP or symbol_stop IMO :P
There was a problem hiding this comment.
Stop is too general I think
There was a problem hiding this comment.
I don't really like hungarian notation. Prefixing it with "symbol" doesn't make the intent any clearer.
There was a problem hiding this comment.
Okay. It's stop. Is it a function or what? Can I stop(ky(...))?
There was a problem hiding this comment.
@szmarczak thankfully that will generate a TypeError. But I get your point. I have the same icky feelings about objects with plural names. I tend to reserve plural names for arrays and it can be confusing if there is no distinction in some APIs. Symbols add to the problem because they are often used to signal intent for an action, similar to a function, so the naming styles conflict. TypeScript and IntelliSense make this a bit more obvious, though.
|
Any thoughts on making it a property on the default export instead of a named export? It doesn't really make sense on its own and |
index.test-d.ts
Outdated
| @@ -25,6 +25,7 @@ expectType<typeof ky>(ky.create({})); | |||
| expectType<typeof ky>(ky.extend({})); | |||
| expectType<HTTPError>(new HTTPError(new Response)); | |||
| expectType<TimeoutError>(new TimeoutError); | |||
| expectType<symbol>(stop); | |||
There was a problem hiding this comment.
I'm not sure if it matters, but shouldn't this be Symbol (capitalized)?
There was a problem hiding this comment.
No, it’s a primitive, so should be lowercased (both work, at least until I get XO working with ambient TS files).
I went ahead and made this change. I think it makes sense. |
@sindresorhus I also prefer What do you think about doing this for the other exports as well? I believe it would solve that Rollup warning during the build and make it easier for CommonJS users to import the package, as they should then be able to use |
👍 |
Hello,
This is related to the discussion from #185.
Summary of the PR:
NO_RETRY_SYMBOLbeforeRetryhook in order to abandon retriesthrowHttpErrorsenabled@sholladay @sindresorhus
Thoughts?
Thank you!