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

feature: Add shouldRetry option #75

Merged
merged 10 commits into from
Dec 20, 2023

Conversation

pukuba
Copy link
Contributor

@pukuba pukuba commented Sep 28, 2023

Referencing #67 and #70, I've added the shouldRetry option.

Please review when you have time.

Fixes #70

@pukuba
Copy link
Contributor Author

pukuba commented Sep 29, 2023

I've checked the comments on the issue. Here are my thoughts.

In pRetry, using the shouldRetry method is beneficial for reducing the complexity of retry logic, enhancing code readability, and promoting reusability.

  • Reducing Complexity: The shouldRetry method allows for detailed error handling, enabling specific conditions for retrying. This separation of concerns simplifies the overall structure of the code.
  • Enhancing Code Readability: With shouldRetry, the retry logic is clear and concise, making the code easier to read and understand. It allows developers to easily discern under what conditions a retry will be attempted.
  • Promoting Reusability: The shouldRetry method can be reused across different parts of the application, ensuring consistency and reducing the need for redundant code.

Certainly, using bail is a good method, but I also think adding a shouldRetry option to p-retry is a good approach.

Respecting the code owner's opinion. If you have any additional thoughts, please leave a comment! @sindresorhus

@sindresorhus
Copy link
Owner

Not sure I trust this pull request. Your comment is clearly ChatGPT, so I'm going to assume the code changes are too.

@pukuba
Copy link
Contributor Author

pukuba commented Oct 17, 2023

I didn't use GPT to write code, but I used it to write PR because I'm not good at English.
The comment I wrote before contains my thoughts.

If you want to close, you can close. Feel free to give me your opinion.

index.js Outdated Show resolved Hide resolved
index.d.ts Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
test.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

You need to add it to the readme too.

readme.md Outdated

Decide if a retry should occur based on the error. Returning true triggers a retry, false aborts with the error.

shouldRetry is not called for TypeErrors (except network errors) and AbortError.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
shouldRetry is not called for TypeErrors (except network errors) and AbortError.
It is not called for `TypeError` (except network errors) and `AbortError`.

Copy link
Owner

Choose a reason for hiding this comment

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

index.d.ts too

@sindresorhus sindresorhus merged commit b993a96 into sindresorhus:main Dec 20, 2023
3 checks passed
@madmed88
Copy link

is it possible to backport this to v4?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to customize whether an error should be retried
3 participants