senny / sablon

Ruby Document Template Processor based on docx templates and Mail Merge fields.
MIT License
443 stars 126 forks source link

Lazy load image content #137

Open eneagoe opened 5 years ago

eneagoe commented 5 years ago

Instead of reading the image content from files/objects at declaration, use a lambda to delay reading the image data only when the image is actually inserted in the template.

The main reason for this would be to save some memory: we have a scenario when we need to make a lot of images available to a template renderer, but the templates are dynamically generated and we don't know upfront which images (if any) will be used by the templates.

stadelmanma commented 5 years ago

Hi @eneagoe I think this is a good idea, but the one challenge I can think of it the opposite case. How would this affect performance if you only had a few images that needed reused hundreds, thousands etc. of times. Also, I haven't tested it but it looks like this code might break if an image is used twice in the same template, even if it doesn't you are re-reading the image every time it's used.

There is also a degree of added complexity and, while I don't know, I doubt this will be a common problem among the entire user base.

That all being said I see two possible routes. The first is that you register your own "LazyImage" content type inside your application to wrap this type of logic. In which case you'd simply make "data" a method call instead of an attribute to avoid needing to change code in operations.rb. Using ||= you could cache the image data for future use instead of reading every time.

Alternatively, we could consider merging this PR if the "lazy loading" was toggle-able via a keyword passed in when setting the image up in the context hash.

eneagoe commented 5 years ago

Regarding using the same image twice in the same template, we have this scenario in our app, and there seems to be no problem there - the image is correctly inserted every time.

Also, I might not be correct, but the code at https://github.com/senny/sablon/blob/4f6a059865cdace5824875384587cf016fd5ac3a/lib/sablon/operations.rb#L133 seems to cache the image the first time it's being generated, so it is being re-used every time it's referenced afterwards.

We did consider having a separate LazyImage handler, but in the end the current set of changes seemed more easy to maintain. However, it is up to you - we could try the keyword argument for explicitly requesting lazy loading, or even implementing a new LazyImage handler.