wicked_pdf
wicked_pdf copied to clipboard
Try to fetch asset from filesystem first before potentially going over the network
Context
Currently for some rails configuration, the read_asset
method (used in methods like wicked_pdf_stylesheet_link_tag
, allowing to inline CSS/JS inside pdf template) goes over the network to fetch assets.
For example, when assets are precompiled and Rails is configured to use a specific asset host (a CDN for example), the read_asset
method retrieves the file through a HTTP call to the CDN.
This may be a waste of resources in cases when the asset is still available on the filesystem.
Moreover, the underlying HTTP library used (Net::HTTP
) is not raising any exception if the response code is > 299.
If for any reason you get a 3XX (the library is not following redirects either), 4XX, 5XX, the css/js included in the pdf may be empty/invalid. The generated pdf will be (silently!) completely messed up.
Intent
This PR attempts at trying to improve this by first trying to retrieve the asset from the filesystem (generally the pre-compiled asset in the public folder) and read it if available, otherwise falling back on the usual mechanism if the asset is not found.
I am far from clearly aware of the consequences of this change, but wanted to illustrate my reasoning.
Any advice/feedback welcome!
Also, fixing the issue with the silent errors might also be something worth considering.
Notes
I didn't manage to make the tests pass on master. I had those two failures:
Failure:
WickedPdfHelperAssetsTest#test_wicked_pdf_stylesheet_link_tag_should_inline_the_stylesheets_passed_in [workspace/wicked_pdf/test/functional/wicked_pdf_helper_assets_test.rb:14]:
--- expected
+++ actual
@@ -1,3 +1,2 @@
"<style type='text/css'>/* Wicked styles */
-
</style>"
rails test workspace/wicked_pdf/test/functional/wicked_pdf_helper_assets_test.rb:12
......F
Failure:
WickedPdfHelperAssetsTest#test_wicked_pdf_javascript_include_tag_should_inline_the_javascripts_passed_in [workspace/wicked_pdf/test/functional/wicked_pdf_helper_assets_test.rb:26]:
--- expected
+++ actual
@@ -1,3 +1,2 @@
-"<script type='text/javascript'>// Wicked js
-;
+"<script type='text/javascript'>// Wicked js;
</script>"
rails test workspace/wicked_pdf/test/functional/wicked_pdf_helper_assets_test.rb:24
I guess it is probably my local configuration? Anyway the change did not change the result (exactly the same two failures), but I guess those test cases are actually the one we want to pass.
I am not sure what test to add in this case, and also not sure the changes I made are already covered by tests.
@Mathiou04 if you rebase with master, we might see the failures if any on Github actions
Hi @mathieujobin Thanks! This is done, waiting for approval.
after I removed the trailing space, and merged with latest master
all tests passed https://github.com/payrollhero/wicked_pdf/actions/runs/2372354673