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

feat: add RetryRequestError + add error to the context for BC #1443

Merged
merged 10 commits into from
Aug 11, 2022

Conversation

vladfrangu
Copy link
Member

The PR adds the error to the crawling context for the error handlers.

Since you can throw anything, the correct type for errors in the error handlers is unknown instead of Error, so this PR fixes that too.

@vladfrangu vladfrangu requested a review from B4nan August 5, 2022 10:43
packages/basic-crawler/src/internals/basic-crawler.ts Outdated Show resolved Hide resolved
@@ -33,6 +33,7 @@ import {
Statistics,
purgeDefaultStorages,
validators,
RetryRequest,
Copy link
Member

Choose a reason for hiding this comment

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

I haven't noticed before, but shouldn't this be named RetryableError or RetryRequestError or something?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, should be *Error to make sure people understand its something to throw, not to return.

I do like RetryRequestError name better, but would go with RetryableError as its consistent with NonRetryableError we already have.

packages/basic-crawler/src/internals/basic-crawler.ts Outdated Show resolved Hide resolved
packages/basic-crawler/src/internals/basic-crawler.ts Outdated Show resolved Hide resolved
packages/basic-crawler/src/internals/basic-crawler.ts Outdated Show resolved Hide resolved
@@ -33,6 +33,7 @@ import {
Statistics,
purgeDefaultStorages,
validators,
RetryRequest,
Copy link
Member

Choose a reason for hiding this comment

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

Yes, should be *Error to make sure people understand its something to throw, not to return.

I do like RetryRequestError name better, but would go with RetryableError as its consistent with NonRetryableError we already have.

packages/basic-crawler/src/internals/basic-crawler.ts Outdated Show resolved Hide resolved
packages/basic-crawler/src/internals/basic-crawler.ts Outdated Show resolved Hide resolved
packages/basic-crawler/src/internals/basic-crawler.ts Outdated Show resolved Hide resolved
packages/core/src/errors.ts Outdated Show resolved Hide resolved
packages/core/src/errors.ts Outdated Show resolved Hide resolved
packages/basic-crawler/src/internals/basic-crawler.ts Outdated Show resolved Hide resolved
@B4nan B4nan changed the title fix: add the error to the context in error handlers and correct the type of the error to unknown feat: add RetryRequestError + add error to the context for BC Aug 11, 2022
@B4nan B4nan merged commit 1a54171 into master Aug 11, 2022
@B4nan B4nan deleted the fix/augment-failed-request-handler-ctx-with-error branch August 11, 2022 07:36
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