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 support for sameDomainDelay #2003

Merged
merged 21 commits into from Jul 31, 2023

Conversation

Dineshhardasani
Copy link
Contributor

@Dineshhardasani Dineshhardasani commented Jul 21, 2023

Implemented a new feature to introduce a delay while crawling same domain requests.

closes #1993

@Dineshhardasani Dineshhardasani changed the title Added Support for sameDomainDelay feat: Added Support for sameDomainDelay Jul 21, 2023
@Dineshhardasani
Copy link
Contributor Author

@B4nan As of now, I have attempted to add functionality for introducing a delay in same domain requests. Could you please review these changes?

Copy link
Member

@B4nan B4nan left a comment

Choose a reason for hiding this comment

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

thanks, left a few initial comments before a deeper review - please focus on removing the unrelated changes and fixing the linter issues, so the produced diff makes more sense.

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/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/basic-crawler/src/internals/basic-crawler.ts Outdated Show resolved Hide resolved
@B4nan
Copy link
Member

B4nan commented Jul 21, 2023

also do the rebase so we can run the tests, i have a hunch this will break them. which brings me to the next step - we need tests for this as well

@Dineshhardasani
Copy link
Contributor Author

Added fix for above comments and rebased branch. @B4nan

@B4nan
Copy link
Member

B4nan commented Jul 24, 2023

thanks for the changes, this looks much better! will take a closer look later today or tomorrow

@B4nan B4nan changed the title feat: Added Support for sameDomainDelay feat: add support for sameDomainDelay Jul 27, 2023
Copy link
Member

@B4nan B4nan left a comment

Choose a reason for hiding this comment

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

looking pretty good, last thing is to add a test case

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/basic-crawler/src/internals/basic-crawler.ts Outdated Show resolved Hide resolved
@B4nan B4nan requested a review from barjin July 27, 2023 15:34
@B4nan
Copy link
Member

B4nan commented Jul 31, 2023

I will merge this as I want to release a new minor version this morning, but we still want to see some tests for this new feature, so please PR that separately.

@B4nan B4nan merged commit e796883 into apify:master Jul 31, 2023
7 checks passed
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.

Support for SameDomainDelay And RetryDelay in Crawling
3 participants