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

Expose Cache #231

Merged
merged 20 commits into from
Feb 24, 2020
Merged

Expose Cache #231

merged 20 commits into from
Feb 24, 2020

Conversation

sergiodxa
Copy link
Contributor

@sergiodxa sergiodxa commented Jan 14, 2020

This attempts to partially resolve #212 and resolves #161 and resolves #237 and resolves #158

Implementation Notes

I updated this to only expose the global cache, I kept the cache interface, adding a way to tell the set, delete and clear methods if you want to notify components reading from the cache, this way mutate can call cache.set and cache.set can also call mutate as a notification method without running on a infinite loop (mutate calls cache.set with this flag as false).

I did this to come back to custom cache once I found a way to let mutate know which cache to call without manually passing it.

I created a CacheInterface and make the global config create a default instance of the cache, also exported both the interface and the Cache class.

I also updated useSWR to read from the cache instance received through the context object.

I was not able, yet, to find a way to make mutate and trigger read from the correct cache instance, because they are global my only idea so far was to pass the cache instance as a third argument (probably optional), but that could make it hard to test components using a custom cache for tests and a default cache for the mutate call.

Missing things

- useSWRPages is still using global cache (I haven't tried yet to use it here but I believe is the same as mutate and trigger)
- mutate is still using global cache
- trigger is still using global cache

Any feedback is welcomed!

@sergiodxa sergiodxa changed the title Custom Cache Support Expose Cache Jan 19, 2020
@sergiodxa
Copy link
Contributor Author

@quietshu I updated this PR to only expose the cache without letting the user replace it, let me know if you want me to change something, I will try again to allow to replace the cache once I think a good way to let mutate identify the correct cache.

@shuding
Copy link
Member

shuding commented Jan 19, 2020

Thank you @sergiodxa so much for working on this! I read the code briefly and it looks great and very clean 👍 And yeah indeed it will be a tricky problem for global mutation, as well as namespaced cache (if we're gonna do that). I will review the code again carefully and think about that too.

@gabrieledarrigo
Copy link

Any news on this?
It can be an unblocker for #161 that's quite important in regards to testing.

@mirshko
Copy link

mirshko commented Feb 3, 2020

Also curious,

looking to hopefully have it fix #237

@Daiz
Copy link

Daiz commented Feb 9, 2020

This should also resolve #158

@andreoav
Copy link

This PR solves some problems we have with SWR and testing.
Currently we need to skip all tests for components that uses SWR.

@embeddedt
Copy link

embeddedt commented Feb 10, 2020

Having a way to clear cache is also useful for when one component does an API request and you need another component to update what it's displaying and not show its previous data (without having to use ugly callbacks and refs).

@gabrieledarrigo
Copy link

Ciao guys, any update on this?

@sergiodxa
Copy link
Contributor Author

@gabrieledarrigo I’m waiting for Shu to review it

@nulladdict
Copy link
Contributor

Hi, great job on this PR
However, I feel like we also need to expose getErrorKey and getKeyArgs, otherwise we wouldn't know what key to use

@sergiodxa
Copy link
Contributor Author

@nulladdict you are right, I move them to the Cache class as a single serializeKey method

@sergiodxa
Copy link
Contributor Author

@quietshu you are right, it will make more sense, and it will still work if the user pass the serialized string directly, and applied it in b4ed289

@sergiodxa sergiodxa requested a review from shuding February 24, 2020 20:11
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.

Looks great. Let's ship it!

@shuding shuding merged commit bdb4324 into vercel:master Feb 24, 2020
@shuding shuding deleted the custom-cache branch February 24, 2020 20:21
@shuding
Copy link
Member

shuding commented Feb 24, 2020

Released [email protected], will fully test in prod before marking it as stable (v0.2.0) 🙏

@sergiodxa
Copy link
Contributor Author

Makes sense, I will try it in production too

@embeddedt
Copy link

I can also test this with the use case I described above. If I don't reply, assume everything worked fine. 🙂

@embeddedt
Copy link

I tested the feature with my application and it has solved the problem I was having before. Thanks!

@nulladdict
Copy link
Contributor

I've also tested this in prod, and it works pretty fine for us.
One small thing is that for my use case (clearing out all errors in suspense mode when retry button in my ErrorBoundary is clicked) I had to finesse the error prefix from cache.
I've used something like this:

const [_key, , _errorKey] = cache.serializeKey('key')
const errorKeyPrefix = _errorKey.replace(_key, '')

So I think it might've also been useful to expose the hardcoded err@ prefix

@gabrieledarrigo
Copy link

Ciao guys, big up for your work on this!
But, question, does it work with testing?
I'm trying cache.clear in my afterEach block, but it seems that it doesn't work:

import { cache } from 'swr';

afterEach(() => {
  cache.clear()
});

it('should fetch on mount', async () => {
  await act(async () => {
    <Foo id="1" />
  });

  expect(fetch).toHaveBeenCalledWith('1');
});

it('should show an error when fetch fails', async () => {
  (fetch as jest.Mock).mockRejectedValue(new Error('An error occured'));

  await act(async () => {
    mount(
      <Foo id="1" />
    );
  });

  expect(fetch).toHaveBeenCalledWith('1');
  // other assertions that the error message is rendered
});

But the second test doesn't pass because fetch is never called; What am I missing?

@andreoav
Copy link

@gabrieledarrigo I have the same problem, even with cache.clear, swr returns the result from the previous test.

@sergiodxa @quietshu Do we need to do something special to clear the cache during testing?

@sergiodxa
Copy link
Contributor Author

@andreoav @gabrieledarrigo I tried it in a test and found the issue, is because the deduping feature of SWR, since not enough time has passed since the last request SWR is reusing the fetch, you can fix it in two ways:

  1. Add dedupingInterval: 0 to the configs of the useSWR call
  2. Wrap the component to test in SWRConfig and set dedupingInterval: 0 there

I recommend to use the second option so your component will still have deduping of requests enabled while running inside your app and you can disable it only for the test.

I will send a PR with an example soon.

Using the example code above something like this should work:

import { SWRConfig, cache } from 'swr';

afterEach(() => {
  cache.clear()
});

it('should fetch on mount', async () => {
  await act(async () => {
    <SWRConfig value={{ dedupingInterval: 0 }}>
      <Foo id="1" />
    </SWRConfig>
  });

  expect(fetch).toHaveBeenCalledWith('1');
});

it('should show an error when fetch fails', async () => {
  (fetch as jest.Mock).mockRejectedValue(new Error('An error occured'));

  await act(async () => {
    mount(
      <SWRConfig value={{ dedupingInterval: 0 }}>
        <Foo id="1" />
      </SWRConfig>
    );
  });

  expect(fetch).toHaveBeenCalledWith('1');
  // other assertions that the error message is rendered
});

@andreoav
Copy link

@sergiodxa Nice!! I updated my tests and it worked.

I think we should add this to the documentation.

@amannn
Copy link
Contributor

amannn commented Mar 24, 2020

Thanks a lot for your work on this, this is really helpful!

As far as I can tell, this hasn't been released to npm yet, right? Any chance to get a release? Thanks!

@embeddedt
Copy link

@amannn It was released as a beta: #231 (comment)

@amannn
Copy link
Contributor

amannn commented Mar 24, 2020

Ah right, thanks!

@BrunoQuaresma
Copy link

BrunoQuaresma commented Mar 24, 2020

value={{ dedupingInterval: 0 }}

Thanks so much for share this. I think would be nice to have this kind of info on docs.

@p19ky
Copy link

p19ky commented Feb 23, 2021

So is there a way I can access the cache variable? I want to fetch only once and I want a condition like !cache.has(dataKey) or similar

@christiaan
Copy link

christiaan commented Feb 27, 2021

So is there a way I can access the cache variable? I want to fetch only once and I want a condition like !cache.has(dataKey) or similar

@p19ky I use this way of calling swr to only have it fetch once.

  return useSWR<string>(API_PICTURES_IMAGE(pictureId), {
    // Images cannot be changed so should never be revalidated
    revalidateOnFocus: false,
    revalidateOnMount: false,
    revalidateOnReconnect: false,
  });

@p19ky
Copy link

p19ky commented Feb 27, 2021

So is there a way I can access the cache variable? I want to fetch only once and I want a condition like !cache.has(dataKey) or similar

@p19ky I use this way of calling swr to only have it fetch once.

  return useSWR<string>(API_PICTURES_IMAGE(pictureId), {
    // Images cannot be changed so should never be revalidated
    revalidateOnFocus: false,
    revalidateOnMount: false,
    revalidateOnReconnect: false,
  });

When I tried it this way, it does not load once. I does not load at all. So what I did to resolve this is the following:

import { cache } from "swr"; //imported cache that is used by SWR

const {
    data: categories,
    error: errorCategories,
    size: sizeCategories,
    setSize: setSizeCategories,
    isValidating: isValidatingCategories,
  } = useSWRInfinite(getKeyCategories, categoriesFetcher, {
    revalidateOnFocus: false,
    revalidateOnMount: !cache.has(() => getKeyCategories()), //only revalidate if there is nothing in cache for my categories Key
  });

@pke
Copy link

pke commented Feb 1, 2022

I am running into this again and again. Why isn't clearCache not really clearing the deduping cache too?
Wrapping ever single test in SWRConfig is really a chore and makes the test less readable.

Solutions:

  1. export a function to disable deduping globally by default
  2. enhance clearCache to also clear the deduping cache

Vinnl added a commit to Vinnl/swr that referenced this pull request Apr 20, 2022
The Cache has been exposed since [1], which supposedly includes a
.clear() method [2], which in turn is not actually present in the
type definition [3]. This adds it to the type definition to align
with the implementation.

Since Map also contains a .clear() method [4], this shouldn't break
the ability to pass that in as a map.

[1] vercel#231
[2] vercel#161 (comment)
[3] vercel#161 (comment)
[4] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map/clear
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.

put initialData in cache [RFC] Custom Cache Support Have a way to clear cache Expose cache to users