Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial commit of component cache #3

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

kalafut
Copy link

@kalafut kalafut commented Sep 17, 2024

This is a proposed experimental cache package that's a follow-on from the discussion at a-h/templ#910. A couple of notes:

  • I didn't check in the generated template files required for the test (they pollute the docs), so templ generate is required for tests to pass.
  • I don't love the idea of using 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.
  • Happy for API feedback. Though it is reasonably complete with tests and docs, it is also pretty easy to change, too.

Copy link
Contributor

@a-h a-h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. Nice work!

cache/cache.go Outdated Show resolved Hide resolved
cache/cache.go Outdated
Comment on lines 109 to 110
// or the expiration for an individual component. If the duration
// is 0 then there is no expiration.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

cache/cache.go Outdated Show resolved Hide resolved
cache/cache.go Outdated Show resolved Hide resolved
cache/cache.go Outdated Show resolved Hide resolved
cache/cache.go Outdated Show resolved Hide resolved
}

// Render children to a buffer.
var buf bytes.Buffer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@garrettladley garrettladley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remember to also update the docs in templ by creating a new md file here

cache/go.mod Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of creating a new module, should this just be a submodule of github.com/templ-go/x?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kalafut
Copy link
Author

kalafut commented Nov 9, 2024

(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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants