Skip to content

feat(core): provide requests to skipped callbacks#3867

Open
AkhilTrivediX wants to merge 1 commit into
apify:masterfrom
AkhilTrivediX:agent/expose-skipped-request
Open

feat(core): provide requests to skipped callbacks#3867
AkhilTrivediX wants to merge 1 commit into
apify:masterfrom
AkhilTrivediX:agent/expose-skipped-request

Conversation

@AkhilTrivediX

Copy link
Copy Markdown
Contributor

Summary

  • pass the full Request to onSkippedRequest instead of a URL string
  • preserve request metadata for skips caused by filters, robots.txt, limits, crawl depth, and redirects
  • update the crawl-depth coverage to verify userData reaches the callback

Why

onSkippedRequest previously discarded request metadata, which made it impossible for handlers to inspect fields such as userData. This follows the v4 API direction in #3801: callers receive the request object and can read its URL from request.url.

Validation

  • corepack yarn eslint packages/basic-crawler/src/internals/basic-crawler.ts packages/browser-crawler/src/internals/browser-crawler.ts packages/core/src/enqueue_links/enqueue_links.ts packages/core/src/enqueue_links/shared.ts packages/http-crawler/src/internals/http-crawler.ts packages/playwright-crawler/src/internals/adaptive-playwright-crawler.ts test/core/crawlers/basic_crawler.test.ts
  • corepack yarn tsc-check-tests
  • corepack yarn vitest run test/core/crawlers/basic_crawler.test.ts --silent (68 passed)

Closes #3801

@barjin barjin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you @AkhilTrivediX !

See my comment below regarding the interface change, I'm otherwise fine with the changes 👍


Btw another thing to consider is the larger memory footprint (see screenshot).

Image

This is imo expected (as we're constructing Request instances now) and likely wouldn't be as striking in average cases (I ran the benchmark on a skip-heavy scenario), so it's not a blocker for me. Still, something to keep in mind.

Thanks!

export type SkippedRequestReason = 'robotsTxt' | 'limit' | 'enqueueLimit' | 'filters' | 'redirect' | 'depth';

export type SkippedRequestCallback = (args: { url: string; reason: SkippedRequestReason }) => Awaitable<void>;
export type SkippedRequestCallback = (args: { request: Request; reason: SkippedRequestReason }) => Awaitable<void>;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a breaking change for users passing onSkippedRequest: ({ url }) => { ... }.

If we agree on this interface, this PR needs to be targeting the v4 branch (the upcoming major). Even then, I'd prefer to only deprecate the url field (perhaps with a warning message informing about the new request field?

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.

Provide Request instance in onSkippedRequest callback

4 participants