Perform the error handling for raise_request_on_failer before waiting for selectors / functions
#202
Conversation
…ng for selectors
There was a problem hiding this comment.
Thanks for the PR @feliperaul however I can see that this would negatively impact people expecting the current behaviour. Although I've outlined a solution that should work for both.
Before merging I'd also like to see a spec that validates this behaviour. Take a look at the existing ones in spec/grover/processor_spec.rb. It could try request something that should fail, but also specify 'waitForSelector' => '#never-going-to-exist' or similar, then check that the request fails with the 4xx error and not a timeout. Have a crack at it and let me know if you need help 😄
| function RequestFailedError(errors) { | ||
| this.name = "RequestFailedError"; | ||
| this.message = errors.map(e => { | ||
| if (e.failure()) { | ||
| return e.failure().errorText + " at " + e.url(); | ||
| } else if (e.response() && e.response().status()) { | ||
| return e.response().status() + " " + e.url(); | ||
| } else { | ||
| return "UnknownError " + e.url() | ||
| } | ||
| }).join("\n"); | ||
| } | ||
|
|
||
| RequestFailedError.prototype = Error.prototype; |
There was a problem hiding this comment.
I think we could move this up outside of this function
| @@ -17,6 +17,28 @@ const path = require('path'); | |||
| const _processPage = (async (convertAction, urlOrHtml, options) => { | |||
| let browser, errors = [], tmpDir; | |||
|
|
|||
| const handleErrors = () => { | |||
There was a problem hiding this comment.
not so sure about this method name.. handleErrors is a little nondescript. Maybe handleRequestErrors ?
the errors variable probably should have been named in a similar fashion!
| @@ -172,6 +194,8 @@ const _processPage = (async (convertAction, urlOrHtml, options) => { | |||
| await page.goto(displayUrl || 'http://example.com', requestOptions); | |||
| } | |||
|
|
|||
| handleErrors(); | |||
There was a problem hiding this comment.
I see that the opposite also needs to hold true. ie if the wait_for_selector/wait_for_function is waiting for some async data loading to complete and you want to have an error alerted if/when those have completed. This would be a breaking change for anyone expecting the current behaviour 😉
It looks like if we checked for errors right away AND after the delayed checks it should resolve your use case whilst still allowing the behaviour to retain it's existing function.
According to the docs:
However, as it is, the error will only be raised after the
wait_for_selectororwait_for_functionare performed.This means that, for example, if grover hits a 401 (Unauthorized) response when visiting a URL, it will not raise immediately; instead, it will time out because the selector wasn't found.
This pull request extracts the error handling to a separate function, and invokes it after the page navigation, so that if
raise_on_request_failureistrue, an error will be raised without having to wait for a selector/function that will inevitably never happen.