Add support for caching parallel requests

[emilbonnek]

Describe the bug
The caching doesn't seem to work when several requests get kicked off at once in a next js project.

To Reproduce

1. Create a Next js page and inline an SVG there 2 places with cacheRequests={true} and onLoad={(src, hasCache) => console.log(src, hasCache)}
2. Create another page and inline that same SVG 1 place
3. start next dev server and open the first page and then navigate to the second and back to the first
4. You will see the following logs:

my-svg false
my-svg false

Navigation to second page

my-svg true

Navigation to the first page

my-svg true
my-svg true

Expected behavior

my-svg false
my-svg true <- This one should cache

Navigation to second page

my-svg true

Navigation to the first page

my-svg true
my-svg true

[gilbarbara]

Hey @emilbonnek
This has already been discussed in #197

[emilbonnek]

They are talking about several different svgs in multiple sessions, I am talking about a single svg in one session.

I don't think it is the same thing that is being discussed. I already saw their issue before I made this.

[gilbarbara]

The cache is only populated after one of the requests succeed.
So when you request both at the same time there no saved data for the file.
You can set a variable with the duplicated SVG so it will be requested only once...

[emilbonnek]

Yes, it doesn't store it in the cache until it has a successful response. So when users use the same svg multiple times on page the svg ressource will be fetched several times. This is the bug I specified above.

I realize it can be solved in user-land, but wouldn't you consider this a worthwhile feature request for this library as well? Since it is already doing caching, I think it should also be able to avoid ressources being requested in parallel.

[gilbarbara]

Hey @emilbonnek
It's not a bug since it wasn't designed to do that. Since the user can avoid this situation, I don't want to increase the library size to handle this right now.

I'll convert this issue to discussions and gather user feedback.

[emilbonnek]

I will continue to refer to this as a bug - despite not being part of the design.

I expected the caching to work similarly to how the browser caches resources fetched declaratively with an img, object or video tag. The browser will not make 20 round trips to fetch an image ressource just because its included 20 times in an img tag on a page. I think this is the expectation people will have from this library, but I imagine few notice that it actually doesn't.

[gilbarbara]

Humm, ok.

[SzymonGalazka]

It's quite relevant in my case to have this parallel caching too - could you point me in the right direction on how to achieve it if that's not on a library roadmap? @emilbonnek @gilbarbara

[dtrucs]

Maybe there's something to be done in the load method and wait for the fetch response to make a decision.

Rework this part (https://github.com/gilbarbara/react-inlinesvg/blob/v3.0.2/src/index.tsx#L269):

if (cache && cache.status === STATUS.LOADED) {
  this.handleLoad(cache.content, true);

  return;
}

By something like this

if (cache && [STATUS.PENDING, STATUS.LOADING, STATUS.LOADED].includes(cache.status)) {
  if (cache.status === STATUS.LOADED) {
    this.handleLoad(cache.content, true);

    return;
  }

  setTimeout(() => this.load(), 100);

  return;
}

I know that setTimeout is never the best solution, but it could avoid consuming useless requests in this case.

[gilbarbara]

Hey @emilbonnek @SzymonGalazka @dtrucs

I finally got some time to work on caching.

I needed to move the caching to another module so every component instance shares it and can be manipulated and create a polling flow so the cache waits to resolve if there's already a request for the same file.

You can check the implementation here

While doing that, I started playing with the Cache API, which works great.
But there are a few drawbacks:

• bundle size: +2kb (instead of just 1kb for the "simple" cache
• the cached files will be cached forever since the Cache API by itself doesn't care about headers

So it isn't worth implementing it right now. Thoughts?

[gilbarbara]

Multiple instances cache and browser permanent cache added in v4.x