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

Can't make it work with custom storage #383

Closed
lukaszmn opened this issue Oct 19, 2022 · 9 comments
Closed

Can't make it work with custom storage #383

lukaszmn opened this issue Oct 19, 2022 · 9 comments
Assignees
Labels
question Further information is requested

Comments

@lukaszmn
Copy link

Describe the bug

I read all the docs, all examples, all Google and most of the issues ;) And I still can't make custom storage work.

To Reproduce

For example (I took the code from an other issue yet it works the same as my original code that used file system):

my-cache.js:

const CACHE = new Map();
const STORAGE = buildStorage({
  find: (key) => {
    const found = CACHE.get(key);
    if (found?.data) {
      return { ...found, data: JSON.parse(JSON.stringify(found.data)) };
    }
    return found;
  },
  set: (key, value) => {
    console.log(value); // HERE
    if (value?.data) {
      CACHE.set(key, {
        ...value,
        data: JSON.parse(JSON.stringify(value.data)),
      });
    } else {
      CACHE.set(key, value);
    }
  },
  remove: (key) => {
    CACHE.delete(key);
  },
});

const INSTANCE = setupCache(axios.create(), {
  ttl: 2000,
  storage: STORAGE,
});

function getAxiosWithCache() {
  return INSTANCE;
}
module.exports = { getAxiosWithCache };

Usage:

const { getAxiosWithCache } = require('./my-cache');
const axios = getAxiosWithCache();

async function parseSearchPage(url) {
  const res = await axios.get(url);
  const html = res.data;
  console.log(html); // HERE 2
}

Actual behavior

HERE 2 reports the downloaded URL's content.

HERE reports only:

{
  state: 'loading',
  previous: 'empty',
  data: undefined,
  createdAt: undefined
}

Expected behavior

HERE reports the downloaded URL's content, just as HERE 2 does

Additional context

  • Axios: v0.27.2
  • Axios Cache Interceptor: v0.10.7
  • What storage is being used: custom
  • Node Version: v16.16.0
@lukaszmn lukaszmn added the bug Something isn't working label Oct 19, 2022
@arthurfiorette
Copy link
Owner

Hey @lukaszmn, seems that the issue here is how the response is being parsed with their headers and checks to cache or not...
Can you build a reproductible example so I can debug it out on my PC?

@lukaszmn
Copy link
Author

Sure, here it is:

package.json:

{
  "main": "index.js",
  "dependencies": {
    "axios": "^0.27.2",
    "axios-cache-interceptor": "^0.10.7"
  }
}

index.js:

const axios = require('axios').default;
const { buildStorage, setupCache } = require('axios-cache-interceptor');

const CACHE = new Map();
const STORAGE = buildStorage({
  find: (key) => {
    const found = CACHE.get(key);
    if (found?.data) {
      return { ...found, data: JSON.parse(JSON.stringify(found.data)) };
    }
    return found;
  },
  set: (key, value) => {
		console.log(value);
    if (value?.data) {
      CACHE.set(key, {
        ...value,
        data: JSON.parse(JSON.stringify(value.data)),
      });
    } else {
      CACHE.set(key, value);
    }
  },
  remove: (key) => {
    CACHE.delete(key);
  },
});

const INSTANCE = setupCache(axios.create(), {
  ttl: 100000,
  storage: STORAGE,
});

(async () => {
  const urlThatWorks = 'https://www.google.com/';
  const urlThatFails = 'https://www.urbanity.pl/';

  console.log('-----------------------------------------------------------');
  const res1 = await INSTANCE.get(urlThatWorks);
  console.log(res1.data.length);

  console.log('-----------------------------------------------------------');
  const res2 = await INSTANCE.get(urlThatFails);
  console.log(res2.data.length);
})();

Result:
urlThatWorks => state loading & cached
urlThatFails => only state loading

@arthurfiorette
Copy link
Owner

urbanity.pl returns a cache-control: no-cache, private header, and header interpretation is set by default. You could've caught it by enabling development mode.

image.

The remove: (key) => { CACHE.delete(key); }, function gets called once, with the "urlThatFails" respective key, as soon the header interpreter is called:

const expirationTime = axios.headerInterpreter(response.headers);

which calls:

if (noCache || noStore) {
return 'dont cache';
}

Its not a storage problem, we are just respecting what the webserver told us to do: never cache GET https://www.urbanity.pl/ calls. If you want to disrespect that, you can disable the header interpretation for this particular request or override the interpreter on your own.

Feel free to reopen this PR if you need.

@arthurfiorette arthurfiorette added invalid This doesn't seem right question Further information is requested and removed bug Something isn't working labels Oct 19, 2022
@lukaszmn
Copy link
Author

Thank you for the detailed explanation, I totally agree with this behavior.

I did not notice it earlier even though I thought I had enabled the debug mode - I set only debug: console.log, but I did not notice I should have also changed the import path to .../dev. Thanks again.

@ahn-nath
Copy link

@arthurfiorette
Do you recommend using a data structure like a map to persist data across sections and time? Or is it necessary to use a more elegant alternative such as Node.js Redis? When do you think it would be appropriate to use a map, like @lukaszmn did, and when not.

I need the caching to work across sessions and live after closing the window, at least after the ttl expires.

@arthurfiorette
Copy link
Owner

Hey @ahn-nath.

As you are doing this on the client side, Redis or any other cache that requires another request (http or not) doesn't seem logical. As you are just moving the amount of requests problem to another side of your codebase.

Each request access the storage multiple times, so any in-memory work is more than sufficient. Seems that, for your use case, the built in web storage is going to, with local or session storage, do automatically for you.

@arthurfiorette arthurfiorette removed the invalid This doesn't seem right label Nov 5, 2022
@ahn-nath
Copy link

ahn-nath commented Nov 7, 2022

I think I did not explain myself correctly.

I am not doing this on the client side, I am doing it with Node.js. The client side will then use the server side as an API, with axios, but on the backend, I am using the caching method/your library.

The web storage does not work. The two options offered by the option are bound to the user session, or to the browser window. That is, when the user closes the tab or the window, the memory will no longer be cached. I need a persistent memory that will last across different user sessions (all users) and even if they close the tab or window. That is why I chose a custom storage options and was wondering about when you would recommend a library like Redis vs using a data structure, such as in the example the @lukaszmn used.

Sorry for the delayed response. I was travelling.

@arthurfiorette
Copy link
Owner

As it is a backend infraestructure, users refreshing pages and so shouldn't matter at all. As cache is a more a matter of performance, it is hard to describe a useful solution without knowing more about your clients.

I'd start with just an in memory storage and if, for some reason, the amount of data in memory becomes too big to nodejs handle without hassle, then, and just then, I'd migrate to an external solution like Redis. But, if you are already using Redis, why not make a test case and see which one performs better for you use case?

@ahn-nath
Copy link

ahn-nath commented Nov 8, 2022

This makes more sense. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants