fix(basic-crawler): handle request.noRetry after errorHandler

[mstephen19]

See this Discord post to fully understand the use case: https://discord.com/channels/801163717915574323/1019936393235017769

Didn't want to make big changes to existing code so kept the else statement and, therefore,the code repeats itself a little. If a refactor is in order, I can easily fix.

[szmarczak]

Hmm... This should be already working

crawlee/packages/basic-crawler/src/internals/basic-crawler.ts

Line 1067 in f65e3c6

if (request.noRetry || (error instanceof NonRetryableError)) {

[mstephen19]

The code within _canRequestBeRetried() is correct. But preventing a request from being retried conditionally is not working without my fix.

This is because shouldRetryRequest is determined BEFORE the user's provided errorHandler is run. Within their error handler, they might have a conditional statement saying that if the error contains the string "foo", the request should not be retried anymore and they would do request.noRetry = true. BUT we don't check if they did this. We must check if noRetry has been set to true in the errorHandler, and if so, must treat the request as if it is finished. Without this fix, the request will retried once, then the second time around wont be retried again.

If a reproducible example is needed to show what I'm talking about, I can provide one.

@szmarczak

[metalwarrior665]

@mstephen19 Just a note, I would probably not change the if/else flow to if/return. That is a refactoring that should be done separately and here it really messes up the diff (or it was messed by something else?).

[mstephen19]

@metalwarrior665 you know me too well (me and my hatred for else) :P but yeah as I said up above, I only added a few lines of code and a comment. Didn't want to change things and left refactoring for Szymon or Banan to decide. Szymon did make some changes to the PR tho, which I trust.

[B4nan]

else is antipattern, early return ftw :] we need to add eslint rule for this (no-else-return)

[Avaronea]

Review