Feedback

[cevr]

Here's a little app i made for demonstrative purposes: stackblitz.com/edit/vitejs-vite-v6umf3?file=src/App.tsx

After using this library for a couple days, I've gathered some feedback:

Will make a comment for each

[cevr]

Possible to add an invalidate function to the cache?

The implementation I use is:

function invalidate<TParams extends any[], TValue>(
 cache: Cache<TParams, TValue>,
 ...args: TParams
) {
 cache.evict(...args);
 cache.prefetch(...args);
}

[bvaughn]

Initial thoughts: This doesn't seem too useful, if it's literally just wrapping those two methods.

The "useful" thing would be if it also scheduled an update with React– but doing that requires either using a hook (aka useCacheMutation) or manually passing in a "refresh" function from useCacheRefresh (which feels too awkward to be useful IMO).

[cevr]

After thinking about this, I think one thing that is missing from the mutation story is that it expects the result of the mutation to also have the new value for whatever cache. From my experience, it is often that APIs just return some sort of success indicator rather than the updated value. On top of that, unless there is a normalized cache that is able to read the underlying type and schema of the API, it fails to account for other caches that reference that same value. eg: An update to the UserCache does not update UsersCache etc.

There's also the consideration of maybe supporting optimistic updates. Where you supply a sync value of the expected result after the mutation and otherwise rollback if the mutation fails.

This may be why it's not considered too useful for now? In cases like these, calling an invalidate method would be very common

[bvaughn]

Admittedly the current mutation hook is a first pass at supporting mutation 😄 I think the pattern could be expanded.

For instance, "optimistic updates" could be built on top of the mutation hook by combining a sync and async mutation. Sketched out, it might look something like:

const { isPending, mutateAsync, mutateSync } = useCacheMutation(cache);

const onClick = () => {
  mutateSync(params, nextValue);

  mutateAsync(params, async () => {
    const success = await api(...);
    return success ? nextValue : prevValue;
  });
};

// Use isPending to disable UI during the update

An update to the UserCache does not update UsersCache etc.

I think data normalization is a bit larger scope than I'm ready to take on at the moment. You're right though that this package doesn't handle that at all for now.

[cevr]

I agree that data normalization is whole other beast! I like the invalidate function since it follows the pull model more closely.

I also like your example, it's where I would say that it would be helpful. If the mutation is a success, call invalidate for another cache (or caches) while setting the next value for the cache provided in the useCacheMutation

[cevr]

Is it possible to notify subscribers when an eviction happens? Currently no notification is given, unless its an evictAll i believe.

similarly, evicting and forcing an update afterwards does not cause the suspense fallback to show. Intended? You can see it in the app above by going to the posts, and clicking evict current page

[bvaughn]

Ah, yes. This looks like a bug. I've added unit tests for it and fixed it– ab461f1

[cevr]

The current eviction policy seems unpredictable and aggressive, sometimes causing surprising evictions. In the app above, if you go to the posts, you'll see that sometimes when pressing the next button, a complete cache blowout has occurred forcing a suspense fallback. Will also notice it in the users demo (loading a new user will sometimes show all other previously loaded users are now evicted).

I was thinking a possible strategy is to allow the user to provide their own implementation of the internal cacher so to leave the eviction policy to them. I would personally want to use a LRU cache, but some might consider another type of cache. Providing the inversion of control could satisfy all use cases. What do you think?

[cevr]

I think that even if you had the weak refs, it seems oddly aggressive to a point of being detrimental to the user experience. Of the choice between both, I think an LRU allows for a much broader scope of customization on policy than weak refs.

[bvaughn]

I hear what you're saying. This is something I think I'd like to sit on for a bit.

You could disable weak refs and wrap a cache with an LRU to accomplish what you're aiming for, but admittedly that's kind of clunky. I'm concerned it might be complicated for me to support a generic "cache" injection though– because types like map/object/LRU have pretty different semantics than weak ref. Essentially I need to handle weak ref semantics at a lower level, which makes it harder to accept an external param like you're talking about.

[cevr]

Yeah it's definitely a hard problem. My thoughts are like this: if I can't control the eviction policy to some degree, then I won't use it and in this case means disabling the weak refs. At least with an LRU you have many options to use to decide how you want your cache evicted. A generic cache would be an even harder problem no doubt haha

[bvaughn]

Yeah, I hear that. I think I won't do anything on this for now beyond just thinking about it, although I would be open to reviewing a PR if you wanted to put one together.

[cevr]

Okay! I'll research to see what a generic cache would look like, would be a learning experience

[cevr]

I commonly want to know if the current cache item is revalidating (no suspense fallback). What do you think about an API like this:

	const [value, revalidating] = Cache.read()

my current implementation for this is using this helper function:

function useRead<TParams extends any[], TValue>(
  cache: Cache<TParams, TValue>,
  ...args: TParams
) {
  const value = cache.read(...args);
  const status = useCacheStatus(cache, ...args);
  return [value, status === 'pending'] as const;
}

which seems to work

[bvaughn]

I don't think I understand the use case here enough to know if I think it would be widely useful enough to add an API for. (Particularly if you're able to accomplish it with only a few lines of code.) Can you elaborate?

[cevr]

Yeah I think its fine to be honest, a simple preference that can be solved rather easily as you said

[cevr]

provide built in hooks? this one is not really a big deal and philosophically i can understand it not being part of the cache. But something like useCacheStatus(cache, ...) to Cache.useStatus(...) etc

[bvaughn]

I thought about this but it felt a little bloaty (subjective). Something I'll think on a bit more though.

[cevr]

Yeah I agree its very subjective.

[cevr]

the current getValueIfCached throws an error if rejected, which makes it cumbersome in some cases. Thoughts on providing a Cache.peek(...) function that returns the raw record or undefined?

[cevr]

In my implementation i paired it with the function cache.evictIf(predicate, ...args) that was useful in ErrorBoundaries to evict if the cache was rejected before reseting the error boundary

[bvaughn]

I don't want to expose the raw Record because I want to preserve the option of changing that structure (like I did in e647dfd).

It sounds like you're almost looking for the value returned by useImperativeCacheValue, but maybe without the side effect of kicking off a fetch?

Thoughts on providing a Cache.peek(...)

So more to the point– I'm not sure I see the value of this over the current API, which lets you imperatively ask if the record is errored (cache.getStatus) before asking for its value. (You could write your own "peek" method pretty easily.)

function peek(cache, ...params) {
  switch (cache.getStatus(...params)) {
    case STATUS_RESOLVED:
      return cache.getValue(...params);
    default:
      return undefined;
  }
}

[cevr]

Fair enough, I think that makes a lot of sense. As you said, creating one's own peek is simple enough using the getStatus

[cevr]

the subscribeToStatus calls the subscription callback immediately causing an unnecessary render on mount when using uSES. Is this intended?

[bvaughn]

Hm, it was intentional. I wouldn't have expected it to cause an unnecessary React render though. I would have expected React to check the getValue callback, see that it hadn't changed, and bail out without rendering.

[cevr]

Yeah those were my thoughts as well but it doesn't seem to be the case. overall not a huge problem either way

[bvaughn]

Are you perhaps just observing strict mode / strict effects behavior (DEV only)?

[cevr]

🤦 you're right

[bvaughn]

Haha, no worries! It gets a lot of people.

[cevr]

the type inference feels lacking, I think the load function should provide all the signatures it needs instead of having to manually supply the generics. Thoughts?

[bvaughn]

Have you tried just not doing that and seeing if it works? I think the only reason your example didn't work is that the function omitted the "options" param (which confused the generics)

https://www.typescriptlang.org/play?#code/JYWwDg9gTgLgBAbzgYQIYGMAWBTAMhVAEwHkwZgIA7AZwBo50ptUZs0ts4BfOAMyggg4AcmoBXamGw1swgNwAoBeirV4Ad1TB4AXjgAKENQBccSmJAAjbFACUcHQD4z2dXAAKAkMGrZ9+pmoIABsAN2x7JzhfGAAVUGwIMRgA7CCw7HojW1tFBVRqAE9KdD4xEvIqPmwYLABVXyh9YEJTNShgSgBzeggyChpTdhx8IlJKmnsEBTg4VE1tOAWUgEYABg3cmbgmGDEoSkRt2Zbaba5FLiUVGngGm2HOPUZmVkf9adngglbq2sx7lAzlwtgoAPRguA2ARQagKQGPAB0TCI+lBELghAgaUo8Gh0HhjSRKMI+gARAVCGTckA

[cevr]

Ah I see. If the CacheLoadOptions are not intended optional in the load function, maybe something like this could convey that?

https://www.typescriptlang.org/play?#code/JYOwLgpgTgZghgYwgAgMKIBYQDwAU5RwC2AzshAB6QgAmZAglIQJ7ZwjMB8ANMgGpwANgFcInZAG8AUMmQJMEABQA3IaIBc-NRF4A6fQAcCxEpvyFSASk3KA9sBoBuGcigQ4NeiWYgEi-bpGFqbI5ibWoVC2RMAkEAAywADWOAIiYsgAPlrpzgC+UlJgzAYo6AhY8bYeAPIGYMC2IGQAvJIuJMAA5iBCmvQARrZQYADK3b2C+c6FRSVlbnCQ5Vh1DU0keMak5FQQtAxMcKzsXLxpouJt0rJdEGAA0hDMAPya-obbIWFWyC3iJDAUFAXWcskE1RommwVQ8PzIlGodGQAG0AvDeCsEpC1o1mgBdTiKFyyAJBEyaWE0eG7JFkNH6DFoBRU3EbfHIF6o9FfTEsnH1PEkDmaEAQZTQFyWP7iXBRGJxRIpbAXDLZVXTQqUAzDMDIGDCXzrEByRbLBRbYK0-bIxgsNgcHg5S7E2S2QUbTSoM0QLFs5qWkznbScKQRLGB0jB9LiG5yDZ6iTIO6PZ68CEeZB5P7Id3GkgzWRuMDCKAmuOyeQVJSqdKaVV6T7BMxfCJ2BztWRd5AAeh7yACJKz3CHi083l8H0CXxbwQicuisQSyVSIayzpQFa7xdLJozNCn5Kjkk6PT6yDFAHdkINdagmkDbIJBNBFJZdKfJnlLGCu3kR7IeT5IUCAJsggLAiAXRYjmCA+liihximTzMO8IDCEQooYQM0C8BBmgQSC0r-OBQIAcg+6aHAE4IMgijoZhF7YbhpFQARQJETKnbbvcu6sb+BTfjMfbIDQtgQGQIC2Hq0BRFAUiEVBWK6FWWCKAA5BB6m8AAjAATAAzLw6kDIIcAYOpP6FCJsnDCQCkcUpCgqQoihAqIvDuToyBeVZUjUT4tEGkaeIUZCABCthPooBAsFhRA4VAKL4rwQxPpoaUvuw0rcUWvFlpIyCZZoXlZgJsygc0eq2AMABWEAIGAMFtHB7jmtWiEuMhzzvLFxzxYlyWpVFggZSN7ggMR4gAAYACQSH1zC6C+UFgBgeTqPNmV5NN5GUWFHiRU+UhCdZ-ZiRJF7SeQTDDFINX1Y1ymqUoEjFV5-6ojpKW+cJ-a2VA9kPQ1TXOS9iHFepcADAg6mfSi33cL9911SDz2uW9I3qB9vAAETUTQuNI1AohWcDT1gxj70kxA8OIzpZOoxT1YuR16HPrwCM-TTjOPaDLPg15nk08LpOOEAA

[bvaughn]

I think it's trickier than that. It is kind of optional. (Not all request types can be aborted.)

[cevr]

Then I think the relaxed one is a great middle ground, though I think putting CacheLoadOptions as the first argument might be better for discoverability. If the request can't be aborted, the first argument can just be ignored

[bvaughn]

I think first vs last parameter is subjective. I prefer it to be last in this case, but I'll give it a bit more thought 😄

[cevr]

not-started seems like an odd status. I believe it means something like cache-miss unless you can set a cache record with not-started? If not, I think undefined could be a better descriptor of cache-miss

[bvaughn]

I dig this.

[bvaughn]

I wonder if "not-found" would be better, semantically?

[bvaughn]

Renamed! befed09

[cevr]

Even better! :D