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

simpler cache key for talk page #571

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

Conversation

adrienpoly
Copy link
Owner

close #566

poke @jmarsh24

Copy link
Contributor

@jmarsh24 jmarsh24 left a comment

Choose a reason for hiding this comment

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

There is an array of watched talks, could we instead just cache a true or false value for watched instead of caching the talk for a user. That way the cache is shared amongst all the users instead of per user. 🧐

@adrienpoly
Copy link
Owner Author

yeah good idea, I look into that this being said I need to check if the user is not somehow required for something else in the _talk partial

@adrienpoly adrienpoly force-pushed the simplify-talk-cache-key branch from 5d6db6a to 6086368 Compare January 16, 2025 07:56
@@ -1,4 +1,4 @@
<%# locals: (talk:) -%>
<%# locals: (talk:, watched:, signed_in:) -%>
Copy link
Contributor

Choose a reason for hiding this comment

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

@adrienpoly I believe the watched boolean alone is sufficient for the cache key, as a talk cannot be marked as watched if the user is not signed in. Including signed_in seems redundant in this case.

Copy link
Owner Author

Choose a reason for hiding this comment

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

if the user is not signed in we currently don't display the button at all. So I still need that information unless we change that and always display the watched button.

That would be a more significant change as for unsigned user clicking on that button should display the signin/up modale.

@adrienpoly adrienpoly force-pushed the main branch 2 times, most recently from 97e3bb3 to 953a362 Compare January 27, 2025 08:53
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.

Increase Cache Hit Performance
2 participants