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

The type on BeforeRetryHook shouldn't contain any optional parameters #1420

Closed
alvis opened this issue Aug 25, 2020 · 8 comments
Closed

The type on BeforeRetryHook shouldn't contain any optional parameters #1420

alvis opened this issue Aug 25, 2020 · 8 comments
Labels
enhancement This change will extend Got features ✭ help wanted ✭
Milestone

Comments

@alvis
Copy link

alvis commented Aug 25, 2020

Describe the bug

In the typescript definition of BeforeRetryHook, error and retryCount are declared as optional, meaning they're potentially undefined. However, it's not true.

export type BeforeRetryHook = (options: NormalizedOptions, error?: RequestError, retryCount?: number) => void | Promise<void>;

Actual behavior

error and retryCount contains the undefined type

Expected behavior

error and retryCount should be just simply RequestError and number

@alvis
Copy link
Author

alvis commented Aug 26, 2020

Sorry I have an oversight on the documentation. It's now clear to me that beforeRetry may be called with only the options parameter when a response is updated. For instant, updating an access token and retry.
See https://github.com/sindresorhus/got#hooksbeforeretry

The implementation can be found below. The hook is passed with only the options but not any error and retry count

for (const hook of typedOptions.hooks.beforeRetry) {
// eslint-disable-next-line no-await-in-loop
await hook(typedOptions);
}

@alvis alvis closed this as completed Aug 26, 2020
@szmarczak
Copy link
Collaborator

szmarczak commented Aug 26, 2020

There is no retryCount. ah wrong hook sorry

@szmarczak
Copy link
Collaborator

Yeah, but I think we could at least add retry count in that case.

@szmarczak szmarczak reopened this Aug 26, 2020
@szmarczak
Copy link
Collaborator

Actually it's correct. It should be missing since it's not classic retrying. But I'm not sure whether we should call beforeRetry hook or not. Anyway, you can make a check and if retryCount is missing then you know it's from afterResponse.

@alvis
Copy link
Author

alvis commented Aug 26, 2020

My 2cents, but maybe it'll make the usage clearer by separating this special case to another hook in the next version? e.g. beforeRetryAfterResponseChange. beforeRetry for a classic automatic retry, and beforeRetryAfterResponseChange for a programmatic retry?

@szmarczak
Copy link
Collaborator

/cc @sindresorhus

@sindresorhus
Copy link
Owner

I agree the current situation could be improved, but I'm not sold on a beforeRetryAfterResponseChange hook either. I'm going to keep this issue open for alternative proposals on how to improve the situation.

@sindresorhus sindresorhus reopened this Aug 29, 2020
@szmarczak szmarczak added this to the Got v12 milestone Sep 17, 2020
@szmarczak szmarczak added enhancement This change will extend Got features ✭ help wanted ✭ labels Feb 17, 2021
@szmarczak
Copy link
Collaborator

Fixed in #1667

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This change will extend Got features ✭ help wanted ✭
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants