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

Feature: Add flag to fail to generate PDF if assets fail to load. #112

Merged
merged 10 commits into from
Jun 2, 2021
Merged

Feature: Add flag to fail to generate PDF if assets fail to load. #112

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e798dd2
Feature: Add flag to fail to generate PDF if assets fail to load.
deanmarano May 27, 2021
bc4a5b0
Update README.md
deanmarano May 27, 2021
b940c3b
Linting.
deanmarano May 28, 2021
2e8d8aa
Apply suggestions from code review
deanmarano May 28, 2021
de9f8ba
Notify of unknown errors accurately.
deanmarano May 28, 2021
043ebf9
Update codeclimate version per warning.
deanmarano May 30, 2021
0f1dc26
Add more tests, rename flag, handle redirects.
deanmarano Jun 1, 2021
d6d65e2
Remove dubgger.
deanmarano Jun 1, 2021
7c681a8
Rubocop.
deanmarano Jun 1, 2021
bc50a1f
Raise errors on 404s.
deanmarano Jun 2, 2021
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
2 changes: 2 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,8 @@ The `wait_for_selector` option can also be used to wait until an element appears

The `wait_for_timeout` option can also be used to wait the specified number of milliseconds have elapsed.

The `request_failure` option can be used to fail generation if assets fail to load.
deanmarano marked this conversation as resolved.
Show resolved Hide resolved

The Chrome/Chromium executable path can be overridden with the `executable_path` option.

Javascript can be executed on the page (after render and before conversion to PDF/image)
Expand Down
31 changes: 30 additions & 1 deletion lib/grover/js/processor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ try {
process.stdout.write("[\"ok\"]\n");

const _processPage = (async (convertAction, urlOrHtml, options) => {
let browser;
let browser, errors = [];
try {
const launchParams = {
args: process.env.GROVER_NO_SANDBOX === 'true' ? ['--no-sandbox', '--disable-setuid-sandbox'] : []
Expand Down Expand Up @@ -124,6 +124,18 @@ const _processPage = (async (convertAction, urlOrHtml, options) => {
page.setGeolocation(geolocation);
}

const failOnRequest = options.requestFailure; delete options.requestFailure;
if (failOnRequest !== undefined) {
deanmarano marked this conversation as resolved.
Show resolved Hide resolved
page.on('requestfinished', (request) => {
if(request.response() && !request.response.ok()) {
deanmarano marked this conversation as resolved.
Show resolved Hide resolved
errors.push(request);
}
});
page.on('requestfailed', (request) => {
errors.push(request);
});
}

const waitUntil = options.waitUntil; delete options.waitUntil;
if (urlOrHtml.match(/^http/i)) {
// Request is for a URL, so request it
Expand Down Expand Up @@ -190,6 +202,23 @@ const _processPage = (async (convertAction, urlOrHtml, options) => {
await page.hover(hoverSelector);
}

if (errors.length > 0) {
function RequestFailedError(errors = []) {
deanmarano marked this conversation as resolved.
Show resolved Hide resolved
this.name = "RequestFailedError";
this.message = errors.map(e => {
if(e.failure()) {
deanmarano marked this conversation as resolved.
Show resolved Hide resolved
return e.failure().errorText + " at " + e.url();
} else if (e.response() && e.response().status()) {
return e.response.status() + " " + e.url();
deanmarano marked this conversation as resolved.
Show resolved Hide resolved
} else {
return "ERR" + " " + e.url()
Copy link
Contributor

Choose a reason for hiding this comment

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

Two things here.

  1. "ERR" seems maybe a tad vague. It seems like timeouts etc would be handled by the e.failure() case and bad responses by the e.response().status() case. What do you see this handling? If it's "unknown" then suggest using language to that point.
  2. "ERR " + e.url() would seem simpler (albeit after addressing point 1.)

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 put this in before the code was properly handling the previous cases. I think the way the code is structured, I would prefer to keep a else condition (so we don't have things in the errors array that don't have error messages) so I'll change this to state it's an unknown error.

}
}).join("\n");
}
RequestFailedError.prototype = Error.prototype;
throw new RequestFailedError(errors);
}

// If we're running puppeteer in headless mode, return the converted PDF
if (debug === undefined || (typeof debug === 'object' && (debug.headless === undefined || debug.headless))) {
return await page[convertAction](options);
Expand Down
18 changes: 18 additions & 0 deletions spec/grover/processor_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,6 +490,24 @@
it { expect(pdf_text_content).to eq "#{date} Hey there http://www.example.net/foo/bar 1/1" }
end

context 'when request failure option is specified' do
let(:url_or_html) do
deanmarano marked this conversation as resolved.
Show resolved Hide resolved
<<-HTML
<html>
<body>
<img src="http://foo.bar/baz.img" />
</body>
</html>
HTML
end
let(:options) { basic_header_footer_options.merge('requestFailure' => true) }
deanmarano marked this conversation as resolved.
Show resolved Hide resolved
it do
expect do
convert
end.to raise_error Grover::JavaScript::RequestFailedError, %r{net::ERR_NAME_NOT_RESOLVED at http://foo.bar/baz.img}
end
end

context 'when wait for selector option is specified with options' do
let(:url_or_html) do
<<-HTML
Expand Down