Skip to content

Conversation

@ernstwi
Copy link

@ernstwi ernstwi commented Mar 17, 2025

Hi again! I wonder if I got this right, something which confused me while reviewing #36 .


For the generic functions, T is the type stored in the cache and V is the type that is returned from fetchFn and GetOrFetch. Since we wrap the fetchFn already in getFetch here to return T:

	wrappedFetch := wrap[T](distributedFetch(c, key, fetchFn))

Doesn't it already have the correct type for storage in the cache? And the result is later unwrapped to V again here in GetOrFetch.

So we shouldn't need to do the same wrap/unwrap process in makeCall and callAndCache?

@viccon
Copy link
Owner

viccon commented Mar 19, 2025

Hi, thank you for this PR! Going through the types has been on my to-do list for a while, but I've been putting it off, hoping that some of the proposals for Go's generics (which feel somewhat lacking) would eventually get merged. If that happened, I’d have to redesign most of the API anyway.

Not being able to express type constraints (e.g., specifying that V must be assignable to T) or allowing methods to have type parameters kinda forced me to create these package-level functions like sturdyc.GetOrFetchBatch, rather than just using the method directly on the client (e.g., client.GetOrFetchBatch) with a type constraint when you're using the cache to store multiple types. We also have to do these runtime type assertions which I feel like a language with generics should be able to do at compile time.

However, since you started looking into this now, I went ahead and cherry-picked this commit to #36 (since it touches the same functions and makes some of the code there redundant). I also simplified the batch functions in the same way and made the wrap function cleaner, as well as added a couple of tests.

@viccon viccon closed this Mar 19, 2025
@ernstwi
Copy link
Author

ernstwi commented Mar 19, 2025

Nice, thanks! Sorry if I'm picking at details too much, mostly trying to get familiar with the code at the moment :)

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.

2 participants