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

Implement throw error with original error and reraise error #19

Closed
wants to merge 1 commit into from

Conversation

indahreforsiana
Copy link

  • implement throw error with the original error
  • add the option reraise error to rethrow the original error when it's reached max attempts

@ibrohimislam
Copy link

@vcfvct It will resolve #17. but it seems conflicting with bfc517b

@ibrohimislam
Copy link

@vcfvct any comment, about this pull request?

@vcfvct
Copy link
Owner

vcfvct commented Sep 25, 2022

@ibrohimislam this PR has conflict from the beginning ~

@ibrohimislam
Copy link

yes, the conflict is because #18. and it's about the same issue.

@ibrohimislam
Copy link

what do you think about the implementation? the conflict is easy to resolve.

Copy link
Owner

@vcfvct vcfvct left a comment

Choose a reason for hiding this comment

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

It looks great overall. Thanks for the contribution.
I left some minor comments, and please fix the conflict. @indahreforsiana

@@ -11,6 +11,7 @@
"declarationMap": true,
"skipLibCheck": true,
"lib": [
"dom",
Copy link
Owner

Choose a reason for hiding this comment

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

any reason we need dom here?

return await fn.apply(this, args);
} catch (e) {
if (i == maxAttempts) {
if (options.reraise) {
Copy link
Owner

Choose a reason for hiding this comment

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

can we rename this option as something like useOriginalError to reduce confusion?

throw e;
}
backOff && (await sleep(backOff));
if (options.backOffPolicy === BackOffPolicy.ExponentialBackOffPolicy) {
Copy link
Owner

Choose a reason for hiding this comment

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

great simplification. 👍

} */
public retryCount: number;
public originalError: Error;
constructor(originalError: Error, retryCount: number) {
Copy link
Owner

Choose a reason for hiding this comment

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

since this is a public/exported class, I guess it would be better to make them optional to keep backward comparability. What do you think?

@vcfvct
Copy link
Owner

vcfvct commented Sep 25, 2022

what do you think about the implementation? the conflict is easy to resolve.

oh, i was not looking much detail as there's conflict.

Just took a look and left some comments.
@ibrohimislam are you the implementer?

@vcfvct
Copy link
Owner

vcfvct commented Jan 17, 2023

@indahreforsiana @ibrohimislam Since this has been pending here for month and the comments are not addressed. I have to implement that myself at #23 .
Closing this for now. Please let me know if you have any concerns.

Thanks for your contribution.

@vcfvct vcfvct closed this Jan 17, 2023
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.

None yet

3 participants