feature: added generate_pdf_from_html string alternative#1
feature: added generate_pdf_from_html string alternative#1pecampelo wants to merge 10 commits intoMassimiliano-solutiontech:mainfrom
Conversation
|
@pecampelo thanks for the PR (that feature was planned so your contribution is very welcome!). I'll review it on the weekend cheers |
|
Putting it as draft since I would like to test the feature a bit. |
…hat was not being used, now all browsers are disabling sandbox for performance
|
I have made a fully functional application using it, it has proven to be a good alternative opposite to reading from disk. Making it ready for review, would appreciate your input |
|
Ok then I hope to find some time today to review it. |
| ## Quick Start | ||
|
|
||
| In order to have a template you must create struct with `PdfTemplate` derive: | ||
| In order to have a template you must create struct with `PdfTemplate` or `PdfTemplateForHtml` derive: |
There was a problem hiding this comment.
You should even add an usage example and update the test_suite as well
There was a problem hiding this comment.
Makes sense! I will be sure to do that in due time
simple_pdf_generator/Cargo.toml
Outdated
| chromiumoxide = { version = "0.5.1", features = [ | ||
| "tokio-runtime", | ||
| ], default-features = false } | ||
| chromiumoxide = { version = "0.5.1", features = ["tokio-runtime", "fetcher"], default-features = false } |
There was a problem hiding this comment.
The fetcher feature should be added only when is stable on the chromiumoxide crate, maybe you can add as an optional feature as well in this crate
simple_pdf_generator/src/lib.rs
Outdated
| options | ||
| }; | ||
| let options = options.build().expect("Invalid browser options."); | ||
| let options = |
There was a problem hiding this comment.
The no_sandbox flag is useful in containered contexts such as LXC and Docker (when the container is unprivileged but you have only the root user inside the container), it should therefore be left as it is
There was a problem hiding this comment.
altered it back to how it was
| } | ||
|
|
||
| pub async fn generate_pdf( | ||
| pub async fn generate_pdf_from_html( |
There was a problem hiding this comment.
A &str is more correct because the parameter isn't mutable. If you prefer you can leave it as is and I will change it before the release
There was a problem hiding this comment.
I would appreciate it if you could
| SimplePdfGeneratorError::BrowserError(format!("Cannot inject the js: {}", e)) | ||
| })?; | ||
|
|
||
| if !template.tables.is_empty() { |
There was a problem hiding this comment.
Why you removed the tables feature if in the derive is still present? I would like to keep this feature in order to have a more coherent behavior across the library
There was a problem hiding this comment.
That makes sense! I removed it to make it work in my tests, but forgot to readd it. Will be readding it
|
I added the comments, feel free to reach me out (even via email: |
2 participants
This PR derived from a need that came about from a project I am currently building, through which I desired to generate a pdf from a html string, without accessing the disk. I only took the
generate_pdffunction and exchanged the html_path for the string that can come from anAWS S3download or something in the liking.I would like to get your input, @Massimiliano-solutiontech, whether you feel the idea makes sense to this package :)