ConsoleTVs / swrev

Framework agnostic stale while revalidate (SWR) data fetching strategy
MIT License
19 stars 5 forks source link

`SWRFunctionState` unclear param and return values #2

Open marekdedic opened 1 year ago

marekdedic commented 1 year ago

Hi, SWRFucntionState is currently a function that takes D | null (in the latest version used by SSWR, it was changed to undefined on master) - I believe the null is there in case there is no value. However, why would the function even be called if there is no value to mutate?

In case it needs to be called, shouldn't the return value also be D | null? Because what I (and I suspect most users) would do is just short-circuit the mutate function and return null...

ConsoleTVs commented 1 year ago

Well, mutation function is to change state from A to B, but as you pointed out, A may not be in the cache. So, if you call the function, it's your responsability to chose how to create B. B might or might not come from A.

What I can't do is falsely annotate A saying it will always be there, because it will not. When you mutate, B should always be constructed. A might not be there to construct it.

marekdedic commented 1 year ago

Great, than I understood it correctly I think :)

Is that really a reasonable that you would want to mutate a state that is currently null and make it into a non-null state B? What I'm arguing for is that if the state A is null, mutate should just be a no-op - so the mutation function wouldn't even be called...

If not so, every mutation function in my codebase would begin with if (state === null) return null; - from my point of view an unnecessary boilerplate...

ConsoleTVs commented 1 year ago

I'm really not understanding your point here. Yes, of course it is possible to mutate even if nothing was on it. Like, think of it, you want to increment a value. If the value is there and it's X, you return X + 1, if X === undefined then you return 1 or 0.

Either way, from an API point of view, getting an element from a hash map (the cache is a map) is always an Optional. eg:

const m = new Map<string, string>();

m.set("foo", "bar")

m.get("foo") // string | undefined => "bar"
m.get("baz") // string | undefined => undefined

Yes, since TS is not that "strict" we can hardcode it and just put a "!" on the API but I don't feel like breaking the type system for the shake of doing it.

marekdedic commented 1 year ago

I'm really not understanding your point here. Yes, of course it is possible to mutate even if nothing was on it. Like, think of it, you want to increment a value. If the value is there and it's X, you return X + 1, if X === undefined then you return 1 or 0.

Haha, yes, I think we're talking at cross-purposes here :D I agree it's possible to mutate a "nothing" value - I'm asking whether it's useful to do so.

I've always used mutate as a better revalidate - say in my app, the user changes some value. I run a network request to update that value in the backend (DB), but to let the user continue working faster, I mutate the local cache in order to not have to wait for revalidation after the update in the backend. In this case, it is not useful for me to mutate a "nothing" value - if the value isn't cached by swr, I don't need to change it, since it's going to be revalidated anyway.

So the pseudo-code for the mutate function could be something like

function mutate(mutateFn) {
  const currentValue = getCurrentCachedValue();
  if (currentValue === undefined) {
    return;
  }
  setNewCachedValue(mutateFn(currentValue));
}

In effect, the mutate function would be a no-op if no value is cached - no ! or type assertions needed, no breaking of the type system.

Now it's quite possible I'm missing some usecase where you really do want to mutate a "nothing" value to a "something" value - if so, I'm just not seeing it.