swr icon indicating copy to clipboard operation
swr copied to clipboard

feat: Keep track `updatedAt` as part of the SWR state

Open icyJoseph opened this issue 2 years ago • 10 comments

Seeding work to keep track of the updatedAt time. #1703

At the top of my head I can think of a few things to do:

  • [x] Are tests exhaustive enough?
  • [ ] Updates to other parts of the SWR API (?)
  • [ ] Documentation

icyJoseph avatar Dec 16 '21 15:12 icyJoseph

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit be0c42e0f27ce0ef4765e02aa54c2dc2ece630cf:

Sandbox Source
SWR-Basic Configuration
SWR-States Configuration
SWR-Infinite Configuration
SWR-SSR Configuration

codesandbox-ci[bot] avatar Dec 16 '21 15:12 codesandbox-ci[bot]

Thank you @icyJoseph — the implementation looks great! One more thing I think we should include in the test is reflecting the updatedAt state change in other hooks, that are using the same key.

And also, I'd like to hear your idea about the name "updatedAt" here. Should we call it "fetchedAt"? Simply because I've been thinking about this "updatedAt" name for a while and it feels that the mutate() calls should be counted as "updates" too. But in some cases their updates are not that easy to track.

shuding avatar Dec 17 '21 17:12 shuding

Thank you @icyJoseph — the implementation looks great! One more thing I think we should include in the test is reflecting the updatedAt state change in other hooks, that are using the same key.

Right, if we have two or more hooks looking at the same key, updatedAt ought to be the same in those. I'll write a test for it.

And also, I'd like to hear your idea about the name "updatedAt" here. Should we call it "fetchedAt"? Simply because I've been thinking about this "updatedAt" name for a while and it feels that the mutate() calls should be counted as "updates" too. But in some cases their updates are not that easy to track.

I see, mutate calls can take many shapes, from a simple call to fetch fresh data, to a controlled call where we are in total control of the new data.

I do agree that fetchedAt would be more appropriate if the intention is to tell the user when was the last time the fetcher was called, rather than the last time the data was mutated.

Here, broadcastState is called twice, src/utils/mutate.ts wouldn't it be enough to get Date.now() before calling broadcast state in these cases?

icyJoseph avatar Dec 18 '21 11:12 icyJoseph

Here, broadcastState is called twice, src/utils/mutate.ts wouldn't it be enough to get Date.now() before calling broadcast state in these cases?

It's a bit tricky, since the updatedAt state only exists locally in the hook, not in the cache. So when a useSWR hook mounts, it can't know what's the last time the data gets updated.

After some research, I think we should change the structure of the cache into "key → { data, error, isValidating, ... }". Currently it is "data_key → data", "error_key → error", ... which is not very extendable.

shuding avatar Dec 22 '21 16:12 shuding

I agree with your last point there.

Just as an update, over the holidays I wrote a test to check that other hooks consuming key should update their updatedAt key to the latest.

For better or worse, the test failed, so I've been making some changes to turn it green.

While I am at, I've come to believe that eitherlastFetchedAt or fetchedAt make much more sense.

icyJoseph avatar Dec 26 '21 16:12 icyJoseph

Now I am back from my break. I see I've been making an error on my tests. Mostly the call to mutate, and expecting that calls to it should reflect similar updatedAt in the rest of hooks.

One more thing I think we should include in the test is reflecting the updatedAt state change in other hooks, that are using the same key.

In this case, we are talking purely about calls to revalidate, for instance, when a hook mounts with `revalidateOnMount, or when the window is refocused, right?

I'll rewrite the two tests that use mutate. I think we are nearly there with this feature.

icyJoseph avatar Jan 04 '22 19:01 icyJoseph

@shuding how do you feel about the PR now? I think we just need to decide a name for the prop:

It would be helpful to introduce a new state updatedAt, which is a timestamp representing the last time we get the data from the fetcher or mutator.

  • updatedAt: As you said before, this implies the mutator changes, and those, at the moment, are hard to catch. I did give it a go at some point, and the code got out of control. I believe a global map/table would be necessary to achieve this.
  • lastFetchedAt: In the current implementation, whenever the re-validate function actually starts a new request, we call Date.now() right after the await statement, update this hook, and broadcast this property to other hooks.

Other names could be fetchedAt, revalidatedAt (though the name doesn't imply fetch that happens on mount).

icyJoseph avatar Jan 07 '22 08:01 icyJoseph

@shuding Alright, then should we wait on your PR first, and then adapt this branch to that?

icyJoseph avatar Jan 13 '22 13:01 icyJoseph

What happened to this PR? I was thinking of writing some similar functionality myself.

nilskaspersson-imtf avatar Apr 18 '23 11:04 nilskaspersson-imtf

sad that it hasn't been implemented.

benevbright avatar Nov 25 '23 22:11 benevbright