Re-using content with images on multiple renders

[brettwgreen]

Put a test here to demonstrate the problem, but I'm not sure where to add a 'fix'.

General issue is that re-using a context object with images fails on subsequent renders (images come up blank) because the IO objects need to be 'rewound' before they can be used a second time.

https://github.com/senny/sablon/pull/182

[senny]

I'm not sure this is an issue that Sablon necessarily needs to be concerned with. The gem renders the IO that you give it, if the given IO is already read, there's nothing more for Sablon to read. I feel this is more of a caller problem.

I can acknowledge that it is something that might trip people up but at the same time, I don't think that Sablon should rewind any IO's or similar. If it's easy to detect that the IO is already exhausted, we could print a warning.

[brettwgreen]

Since we're sending in the content as what seems like a static JSON object, the caller's instinct is that it should just work 🤷

I just figured it may be easy to add a lightweight call somewhere that rewinds all stream/io objects that doesn't feel terrifically invasive to sablon. If that's harder than I think it is, than that's fine. There are certainly workarounds and maybe we can just add a little info on the documentation for this quirk.

[senny]

The conceptional issue I see with calling rewind on the passed context is that this imposes side-effects onto objects given to Sablon. This is not expected and could cause issues with IO objects that might behave differently than a regular File or StringIO buffer.

I think it's preferred that the caller produces a new IO to give to sablon on each request. Could be implemented with a proc or a context object where the method does not memoize the IO.

[brettwgreen]

OK, I think you make a fair point that the IO being rewound is a side effect the caller may not expect.

I think the biggest surprise/gotcha with this is that the images just vanish (i.e. fail to render anything) on subsequent renders with no errors or warnings. Perhaps it would be better for Sablon to throw an error/warning when a Content object's attempt at read/render results in zero bytes. It took me awhile to understand what was going on.

[senny]

Perhaps it would be better for Sablon to throw an error/warning when a Content object's attempt at read/render results in zero bytes. It took me awhile to understand what was going on.

I think this is a fair point and would be welcome as an addition. Printing a warning and indicating how to resolve it should be sufficient to make the user aware of this concern. We might also want to add some documentation about it.