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: createAdaptivePlaywrightRouter utility #2415

Merged
merged 4 commits into from
Apr 10, 2024
Merged

Conversation

janbuchar
Copy link
Contributor

@janbuchar janbuchar commented Apr 10, 2024

@janbuchar janbuchar requested a review from B4nan April 10, 2024 09:17
@github-actions github-actions bot added this to the 87th sprint - Tooling team milestone Apr 10, 2024
@github-actions github-actions bot added the t-tooling Issues with this label are in the ownership of the tooling team. label Apr 10, 2024
import { MissingRouteError } from './errors';
import type { Request } from './request';
import type { Awaitable } from './typedefs';

const defaultRoute = Symbol('default-route');

export interface RouterHandler<Context extends CrawlingContext = CrawlingContext> extends Router<Context> {
export interface RouterHandler<Context extends Omit<RestrictedCrawlingContext, 'enqueueLinks'> = RestrictedCrawlingContext> extends Router<Context> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not very happy about this. Maybe the type bound should be something else entirely?

Copy link
Member

Choose a reason for hiding this comment

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

Isn't this breaking? I guess for people who use Router.create() it could be (as you no longer have the enqueueLinks helper in the context I think?), although they should be using create*Router factories instead.

I would maybe remove those changes and only handle the fact that enqueueLinks are not there for basic crawler only in createBasicRouter(), that's the only place where the change is actually doing something.

But it's fine to keep it this way too I guess, polishing those things could end up breaking and its better to keep that for v4 (where we also want to get rid of the Record type extension).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, the default value should not change, astute observation! If we get that out of the way, relaxing the bound on the type parameter should not break anything, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah makes sense

@B4nan B4nan merged commit cee4778 into master Apr 10, 2024
8 checks passed
@B4nan B4nan deleted the create-adaptive-router branch April 10, 2024 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t-tooling Issues with this label are in the ownership of the tooling team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing create*Router helper for AdaptivePlaywrightCrawler
2 participants