-
-
Notifications
You must be signed in to change notification settings - Fork 667
feat: disable stream cache for library add-ons #581
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
base: main
Are you sure you want to change the base?
feat: disable stream cache for library add-ons #581
Conversation
WalkthroughThe cache selection logic in the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~4 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/core/src/wrapper.ts (2)
243-254: Minor inconsistency: cache invalidation for non-cached streams.The invalidation logic always attempts to delete from
streamsCache, even for library addons where caching is disabled (line 216). Whilst functionally safe, this creates a small inefficiency.Consider conditionally invalidating only when caching was actually used, though the current approach is acceptable.
216-216: Verified:addon.libraryproperty exists and is correctly typed asboolean | undefined.The property is defined in
AddonSchema(packages/core/src/db/schemas.ts line 137) aslibrary: z.boolean().optional(). The logic correctly handles all three states: whentrue(disables cache), whenfalse(enables cache), and whenundefined(treats as non-library, enabling cache by default). This is the intended behaviour since the property is optional and most addons are not library addons.Consider adding a brief inline comment explaining why library addons bypass the cache, as this helps future maintainers understand the business logic.
|
Ah, I just had another idea: introduce an env var for library stream TTL which is also -1 by default. WDYT? |
|
@Viren070 sorry for the ping but what do you think of this
was thinking of just adding another env var e.g. |
|
I'm not really fond of adding an env var specifically to control cache for library addons - I would prefer to have a much more customisable environment variable that allows you to control cache_ttls for any addon, perhaps using hostnames which would satisfy this use case as well as many others. |
interesting idea. the only problematic thing for exactly my use case would be to differentiate st torz from st store I guess 🙈 |
|
Ah right, i guess we could also allow specifying preset ids. so e.g. the value of this supposed environment variable could be:
Although I guess if theres another host which serves multiple addons and neither are available in the marketplace this variable wouldn't work for them, still I think this would work for everyone and still keep the variable simple. |
Closes #580
Simplest way of dealing with that, check proposed solutions there for possible alternatives.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.