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.

Sign up for GitHub

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

Jump to bottom

Perform the error handling for raise_request_on_failer before waiting for selectors / functions #202

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Perform the error handling for raise_request_on_failer before waiting for selectors / functions #202

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
630a80e
Perform the error handling for `raise_request_on_failer` before waiti…
Sep 22, 2023
5f315cf
Merge branch 'Studiosity:main' into main
feliperaul Jun 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 28 additions & 21 deletions lib/grover/js/processor.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,28 @@ const path = require('path');
const _processPage = (async (convertAction, urlOrHtml, options) => {
let browser, errors = [], tmpDir;

const handleErrors = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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!


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;
Comment on lines +34 to +47
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could move this up outside of this function


if (errors.length > 0) {
throw new RequestFailedError(errors);
}
}

try {
// Configure puppeteer debugging options
const debug = options.debug; delete options.debug;
Expand All @@ -34,24 +56,24 @@ const _processPage = (async (convertAction, urlOrHtml, options) => {
args: process.env.GROVER_NO_SANDBOX === 'true' ? ['--no-sandbox', '--disable-setuid-sandbox'] : [],
userDataDir: tmpDir
};

if (typeof debug === 'object' && !!debug) {
if (debug.headless !== undefined) { launchParams.headless = debug.headless; }
if (debug.devtools !== undefined) { launchParams.devtools = debug.devtools; }
}

// Configure additional launch arguments
const args = options.launchArgs; delete options.launchArgs;
if (Array.isArray(args)) {
launchParams.args = launchParams.args.concat(args);
}

// Set executable path if given
const executablePath = options.executablePath; delete options.executablePath;
if (executablePath) {
launchParams.executablePath = executablePath;
}

// Launch the browser and create a page
browser = await puppeteer.launch(launchParams);
}
Expand Down Expand Up @@ -172,6 +194,8 @@ const _processPage = (async (convertAction, urlOrHtml, options) => {
await page.goto(displayUrl || 'http://example.com', requestOptions);
}

handleErrors();
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


// add styles (if provided)
const styleTagOptions = options.styleTagOptions; delete options.styleTagOptions;
if (Array.isArray(styleTagOptions)) {
Expand Down Expand Up @@ -232,23 +256,6 @@ const _processPage = (async (convertAction, urlOrHtml, options) => {
await page.hover(hoverSelector);
}

if (errors.length > 0) {
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;
throw new RequestFailedError(errors);
}

// Setup conversion timeout
if (options.convertTimeout !== undefined) {
options.timeout = options.convertTimeout;
Expand Down