Skip to content
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

feat: Keep track updatedAt as part of the SWR state #1708

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

icyJoseph
Copy link
Contributor

@icyJoseph icyJoseph commented Dec 16, 2021

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:

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 16, 2021

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 be0c42e:

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

@shuding
Copy link
Member

shuding commented Dec 17, 2021

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.

@icyJoseph
Copy link
Contributor Author

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?

@shuding
Copy link
Member

shuding commented Dec 22, 2021

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.

@icyJoseph
Copy link
Contributor Author

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.

src/use-swr.ts Outdated Show resolved Hide resolved
@icyJoseph
Copy link
Contributor Author

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
Copy link
Contributor Author

@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).

Copy link
Member

@shuding shuding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m back too and thank you @icyJoseph, the implementation now looks excellent to me as well as those tests.

Regarding the name I think fetchedAt is still better. Or, I’d like to give it a try to also include the update time of mutations, and keep the name updatedAt (really want to make it polished before releasing as a stable API).

To make that happen, I’ll come up with a PR to change the cache structure to keep all states by one single key.

Does the plan sound good to you?

@icyJoseph
Copy link
Contributor Author

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

@nilskaspersson-imtf
Copy link

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

@benevbright
Copy link

sad that it hasn't been implemented.

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.

4 participants