Feature: Add flag to fail to generate PDF if assets fail to load. #112
Conversation
There was a problem hiding this comment.
Hi @deanmarano the idea is solid and the change is looking great. I've left a few comments but nothing too drastic. Take a look and let me know what you think. 👍
lib/grover/js/processor.js
Outdated
| } else if (e.response() && e.response().status()) { | ||
| return e.response.status() + " " + e.url(); | ||
| } else { | ||
| return "ERR" + " " + e.url() |
There was a problem hiding this comment.
Two things here.
- "ERR" seems maybe a tad vague. It seems like timeouts etc would be handled by the
e.failure()case and bad responses by thee.response().status()case. What do you see this handling? If it's "unknown" then suggest using language to that point. "ERR " + e.url()would seem simpler (albeit after addressing point 1.)
There was a problem hiding this comment.
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.
Co-authored-by: Andrew Bromwich <a.bromwich@gmail.com>
There was a problem hiding this comment.
Getting really close @deanmarano
I've noticed a few bugs as well as a couple of missing specs (ie test the success result when the option is enabled, but no errors.. and test for a 404).
I've also left a comment about the option name itself. I think as it currently stands it's a bit vague about what it'll do when enabled. A slightly more verbose option name should get rid of any of that ambiguity.
There was a problem hiding this comment.
Hi @deanmarano it looks like there's still one test case missing (404). Can you please add that in? Suggest something like https://google.com/invalid.png or similar
|
@abrom It looks like an expired w3.org cert is causing some tests to fail. I would guess they'll fix it relatively quickly? |
|
FYI this has been released in v1.0.1 |
Hi there! Grover has made our lives so much easier, thank you!
I'm hoping to add in a feature to allow PDF generation to fail when an asset fails to load. I've added a test and a line to the README. Any feedback or help getting this in would be greatly appreciated.