Skip to content

Initial commit of component cache#3

Open
kalafut wants to merge 10 commits into
templ-go:mainfrom
kalafut:main
Open

Initial commit of component cache#3
kalafut wants to merge 10 commits into
templ-go:mainfrom
kalafut:main

Conversation

@kalafut
Copy link
Copy Markdown

@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
Copy Markdown
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!

Comment thread cache/cache.go Outdated
Comment thread 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread cache/cache.go Outdated
Comment thread cache/cache.go Outdated
Comment thread cache/cache.go Outdated
Comment thread cache/cache.go Outdated
Comment thread cache/cache.go
}

// Render children to a buffer.
var buf bytes.Buffer
Copy link
Copy Markdown
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
Copy Markdown
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?

kalafut and others added 6 commits September 24, 2024 07:59
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>
Copy link
Copy Markdown
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

Comment thread cache/go.mod
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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