Initial commit of component cache#3
Conversation
a-h
left a comment
There was a problem hiding this comment.
This looks great. Nice work!
| // or the expiration for an individual component. If the duration | ||
| // is 0 then there is no expiration. |
There was a problem hiding this comment.
That's not quite accurate. There's a 100 year expiration date set! It's unlikely someone will keep their web server running for 100 years, but still...
I thought that if the duration was set to 0, then it would be the same as disabling caching, rather than meaning that there's an infinite cache period.
There was a problem hiding this comment.
But... I see that Microsoft have taken the decision that a zero duration is infinite in their API. Not sure how I feel about this.
Seems confusing.
There was a problem hiding this comment.
There is another issue: 0 as disabling caching is potentially useful at the component level based on some conditions. I think it makes sense to remove any special handling here. If you want 0, you get instant expiration. If you want "forever", that's easy enough for the developer to do with their on large constant.
I've changed it accordingly.
| } | ||
|
|
||
| // Render children to a buffer. | ||
| var buf bytes.Buffer |
There was a problem hiding this comment.
This is great for now, but I expect we'd want to use a buffer pool at some point, to reduce the amount of GC work required over time, since each new buffer is an allocation that needs to be cleared up.
There was a problem hiding this comment.
I've not used sync.Pool before. I thought this would be a straightforward change, but when I added it, my concurrency test started failing (not panicking, but rather the data rendered wasn't what was expected). This is the change: kalafut@f7cdafe
Any idea what I'm doing wrong?
Co-authored-by: Adrian Hesketh <a-h@users.noreply.github.com>
Co-authored-by: Adrian Hesketh <a-h@users.noreply.github.com>
Co-authored-by: Adrian Hesketh <a-h@users.noreply.github.com>
garrettladley
left a comment
There was a problem hiding this comment.
remember to also update the docs in templ by creating a new md file here
There was a problem hiding this comment.
instead of creating a new module, should this just be a submodule of github.com/templ-go/x?
There was a problem hiding this comment.
I was taking a page from the organization of https://github.com/gofiber/contrib. I'm assuming this temple-go/x repo serves as a hub for issue management and discoverability, but will ultimately be a collection of disparate modules. It will be important that users can choose different versions per module, especially since this is declared to be potentially unstable code.
|
(Following up after a few months of me being distracted with other things...) Hi. Can this be merged now? Once in I'll update any docs per #3 (review) |
This is a proposed experimental cache package that's a follow-on from the discussion at a-h/templ#910. A couple of notes:
templ generateis required for tests to pass.cache("")to get an instance of the cache object for whole cache (not component) operations. But it was a tradeoff. The component usage in templates is pretty clean, which I think in preferable.