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

Add support for expires_in: when using render with collection: #51579

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jclusso
Copy link

@jclusso jclusso commented Apr 16, 2024

Motivation / Background

When using the following code you'd expect that you can pass expire_in as you can with rendering individual partials.

<%= render(collection: @posts, partial: 'post', cached: true, expires_in: 1.hour) %>

Detail

  • Pass expires_in to write_multi so the cache key is written with the expiration.
  • Added tests.
  • Added documentation.

Checklist

  • This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

- Pass `expires_in:` to `write_multi` so the cache key is written with the expiration.
- Added test.
- Added documentation.
@mpkhr
Copy link

mpkhr commented Apr 16, 2024

Thanks for the PR @jclusso !
Would it be possible to do this on a per-item basis as mentioned in the original thread #33589 (comment) ?

@jclusso
Copy link
Author

jclusso commented Apr 16, 2024

Thanks for the PR @jclusso ! Would it be possible to do this on a per-item basis as mentioned in the original thread #33589 (comment) ?

I don't understand what that means. You'd have to give me an example.

@mpkhr
Copy link

mpkhr commented Apr 16, 2024

I don't understand what that means. You'd have to give me an example.

Much like the cache-key can be specified using a block (i.e. for each item), it would be great to have the same flexibility for the expiration calculation. Since the original thread came from the idea of improving cache efficiency, using a logic "one expiry for all entries" does leave room open for improving the efficiency.

For example, a model could implement a method, say custom_expires_at which than could be called for each post. I think via block is sufficient, as it's the most flexible way and allows also for an inline calculation if desired:

<%= render(collection: @posts, partial: 'post', cached: true, expires_in: -> (post) { post.custom_expires_at } ) %>

Could also think about supporting a symbol, but don't wanna overcomplicate things

<%= render(collection: @posts, partial: 'post', cached: true, expires_in: :custom_expires_at ) %>

With this addition, it is similar to the cache key, which allows a simple switch (cache: true) and also an individual computation cache: -> (post) {...}

@jclusso
Copy link
Author

jclusso commented Apr 17, 2024

I see, I think I'll leave that to someone else to open a PR for. That need seems very niche and will increase the complexity of this change substantially.

I'm also not confident maintainers would support that, whereas I'd be surprised if they wouldn't be receptive to this as it stands.

@mpkhr
Copy link

mpkhr commented Apr 17, 2024

Alright, sounds good to me

@jvillarejo
Copy link
Contributor

jvillarejo commented Apr 17, 2024

Much like the cache-key can be specified using a block (i.e. for each item),

I think that expires_in as block could be useful but that implementation should be available for every cache method that can receive expires_in: as options, in order to be consistent at the whole framework.

As @jclusso said, it would be better for another PR, because it can affect multiple other parts.

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

Successfully merging this pull request may close these issues.

None yet

4 participants